feat: RFC 9421 HTTP Message Signatures for gateway responses (PE-9049)#671
Open
vilenarios wants to merge 5 commits intodevelopfrom
Open
feat: RFC 9421 HTTP Message Signatures for gateway responses (PE-9049)#671vilenarios wants to merge 5 commits intodevelopfrom
vilenarios wants to merge 5 commits intodevelopfrom
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds RFC 9421 HTTP response signing (HTTPSIG) and attestation: new env/config flags, Ed25519 key management, middleware to sign responses, attestation creation/upload to Arweave with Turbo fallback, metrics, OpenAPI/docs updates, docker-compose mounts, tests, and a runtime Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant Server
participant Middleware as HTTPSIG Middleware
participant Disk
Client->>Server: GET /{txId} or /ar-io/info
Server->>Middleware: prepare response (headers/status)
Middleware->>Disk: read private key / keyId
Middleware-->>Middleware: build signature base (status, headers, optional req)
Middleware->>Middleware: sign with Ed25519 private key
Middleware->>Server: set Signature-Input / Signature headers
Server->>Client: send signed response
sequenceDiagram
autonumber
participant Server
participant Config as Startup/Config
participant Disk
participant Observer as Observer Wallet (RSA JWK)
participant Arweave
Server->>Config: start (HTTPSIG_ENABLED)
Config->>Disk: load/generate Ed25519 key & attestation cache
alt observer configured and no cached txId
Config->>Observer: load observer JWK
Config->>Config: create attestation payload & RSA signature
Config->>Arweave: tryTurboUpload / fallback L1 upload
Arweave-->>Config: return txId
Config->>Disk: save attestation + txId
end
Server->>Server: register HTTPSIG middleware with private key/keyId
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (6)
src/routes/ar-io.ts (1)
131-169: Refresh the/ar-io/infoTSDoc for the newhttpsigfield.The handler comment and example still describe only the pre-HTTPSIG response shape, so they now drift from what this endpoint returns.
As per coding guidelines,
**/*.{ts,tsx,js}: Add or improve TSDoc comments when modifying code to enhance documentation.Also applies to: 170-219
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/ar-io.ts` around lines 131 - 169, The TSDoc for the /ar-io/info handler is out of date because the response now includes a new "httpsig" field; update the comment block above the handler in src/routes/ar-io.ts (the /ar-io/info handler comment around the existing example JSON) to document the httpsig property (its shape and when it's present) and add it to the example response JSON so the docs match the actual response; keep the rest of the existing descriptions (wallet, processId, bundlers, rateLimiter, x402) unchanged and follow the existing formatting/style used in the comment.src/lib/httpsig-upload.ts (1)
134-142: Consider adding a timeout for the L1 transaction post.The
arweave.transactions.postcall has no timeout. While this is only called at startup (not on request path), L1 transactions can be slow under network congestion. A timeout would prevent indefinite startup hangs.♻️ Suggested improvement
+ // L1 POST can be slow; use AbortController for timeout + const controller = new AbortController(); + const timeout = setTimeout(() => controller.abort(), 60000); + - const response = await arweave.transactions.post(tx); + let response; + try { + response = await arweave.transactions.post(tx); + } finally { + clearTimeout(timeout); + }Note: This depends on whether
arweave.transactions.postsupports AbortSignal. If not, consider wrapping withPromise.raceand a timeout promise.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/httpsig-upload.ts` around lines 134 - 142, The arweave.transactions.post call (after arweave.transactions.sign using tx and jwkAny) currently has no timeout and can hang startup; add a timeout around the POST: if arweave.transactions.post supports AbortSignal, create an AbortController, pass controller.signal and call controller.abort after the timeout; otherwise wrap the post call in a Promise.race with a timeout-rejecting promise, ensure you cleanly throw a descriptive error like "Timed out posting attestation L1 transaction" on timeout and preserve the existing non-200/202 error handling.src/routes/ar-io-info-builder.ts (1)
89-107: Consider adding TSDoc comments to new interfaces.The existing interfaces in this file have TSDoc comments explaining their purpose. Adding similar documentation to
HttpsigAttestationInfoandHttpsigInfowould maintain consistency and help consumers understand the attestation trust model.📝 Suggested documentation
+/** + * RSA-signed attestation linking an Ed25519 signing key to an Arweave + * observer wallet identity. Enables verifiers to establish trust chain + * from the HTTP signature back to the on-chain gateway registration. + */ export interface HttpsigAttestationInfo { + /** Arweave transaction ID where attestation is permanently stored */ txId?: string; + /** Base64url Arweave address of the observer wallet that signed the attestation */ observerAddress: string; + /** Canonical JSON attestation payload */ payload: string; + /** RSA-PSS-SHA256 signature over the payload (base64url) */ signature: string; + /** RSA public key in SPKI DER format (base64url) */ rsaPublicKey: string; } +/** + * HTTPSIG response signing configuration exposed in the info endpoint. + * When enabled, qualifying responses include RFC 9421 signatures. + */ export interface HttpsigInfo {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/ar-io-info-builder.ts` around lines 89 - 107, Add TSDoc comments for the two new interfaces HttpsigAttestationInfo and HttpsigInfo similar to the rest of the file: describe the purpose of HttpsigAttestationInfo (fields txId, observerAddress, payload, signature, rsaPublicKey and how they represent an HTTPSIG attestation/verification record) and describe HttpsigInfo (enabled, algorithm, publicKey, keyId, optional solanaAddress and optional attestation) including the trust model and when attestation is present; attach the comments immediately above each interface declaration.src/middleware/httpsig.ts (2)
47-49: Consider using constants for header names.The header names
'Signature'and'Signature-Input'are hardcoded here, but constants are already defined insrc/constants.ts(lines 78-81). Using the constants would improve maintainability and ensure consistency across the codebase.♻️ Suggested refactor
+import { headerNames } from '../constants.js'; + // ... res.writeHead = function (this: Response, ...args: any[]) { // Strip upstream signature headers (gateway-to-gateway forwarding) - this.removeHeader('Signature'); - this.removeHeader('Signature-Input'); + this.removeHeader(headerNames.signature); + this.removeHeader(headerNames.signatureInput);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/middleware/httpsig.ts` around lines 47 - 49, Replace the hardcoded header strings in the middleware's signature-stripping block with the shared constants: import and use SIGNATURE_HEADER and SIGNATURE_INPUT_HEADER (the constants defined in constants.ts) instead of literal 'Signature' and 'Signature-Input'; update the import statement in src/middleware/httpsig.ts to include those constants and change the two calls this.removeHeader('Signature') and this.removeHeader('Signature-Input') to this.removeHeader(SIGNATURE_HEADER) and this.removeHeader(SIGNATURE_INPUT_HEADER) so the code uses the centralized header names.
109-112: Consider logging signing errors for observability.The empty catch block silently swallows all signing errors—only the error counter is incremented. While the design choice to not break responses is correct, adding a debug/warn log would help diagnose signing failures in production without impacting request handling.
♻️ Suggested improvement
- } catch { + } catch (err) { // Signing failure must not break the response — fall through unsigned. metrics.httpSigErrorsTotal.inc(); + // Log at debug level to aid diagnosis without flooding logs + // Consider: log.debug('HTTPSIG signing failed', { error: err }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/middleware/httpsig.ts` around lines 109 - 112, The catch block that currently only calls metrics.httpSigErrorsTotal.inc() silently swallows signing errors; update that catch to also log the caught error (without rethrowing) using the module's logger (e.g., processLogger.warn or logger.debug) so we keep the non-failing behavior but add observability; target the catch in src/middleware/httpsig.ts where signing is attempted (the block calling metrics.httpSigErrorsTotal.inc()) and include the error object and contextual text in the log message.src/lib/httpsig.ts (1)
385-391: Consider atomic write for attestation cache.The cache file write isn't atomic—a crash during write could leave a partially written file. While the code handles corrupt cache gracefully (deletes and recreates), using write-to-temp-then-rename would prevent this edge case entirely.
♻️ Suggested atomic write pattern
+import { renameSync } from 'node:fs'; + // Cache to disk const cacheData: CachedAttestation = { ...attestation, ed25519PublicKey: currentPubKey, }; mkdirSync(keysDir, { recursive: true }); -writeFileSync(cachePath, JSON.stringify(cacheData, null, 2)); +const tmpPath = `${cachePath}.tmp`; +writeFileSync(tmpPath, JSON.stringify(cacheData, null, 2)); +renameSync(tmpPath, cachePath);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/httpsig.ts` around lines 385 - 391, The current cache write (built from cacheData and using mkdirSync(keysDir...) then writeFileSync(cachePath,...)) is not atomic and can leave a partial file; change it to an atomic write-to-temp-and-rename pattern: ensure keysDir exists, write the JSON to a temporary file in the same directory (e.g., cachePath + '.tmp' or a unique temp name), fsync the file (or use writeFileSync then fsync via the file descriptor) to flush data, close it, then rename (fs.renameSync) the temp file to cachePath so the replace is atomic; reference cacheData, keysDir, cachePath, mkdirSync, writeFileSync and perform the rename step to guarantee atomicity.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docker-compose.yaml`:
- Line 63: The WALLETS_PATH env is being reused for both the host bind source
and the container lookup which breaks in-container wallet paths; update the
docker-compose service definition so the volume uses the host override (keep the
current ${WALLETS_PATH:-./wallets} as the left side of the volume mapping) but
set the container-side WALLETS_PATH environment value to the fixed in-container
path /app/wallets (i.e., ensure the container ENV WALLETS_PATH points to
/app/wallets while the volume mapping remains
${WALLETS_PATH:-./wallets}:/app/wallets) so code expecting
/app/wallets/<wallet>.json inside the container still works.
In `@docs/envs.md`:
- Around line 303-316: Add documentation for the KEYS_DATA_PATH environment
variable in the HTTPSIG section of docs/envs.md: describe it as a String
(default "data"), explain it maps to the container path /app/data/keys and is
used to persist or relocate the generated HTTPSIG keys (which are referenced by
HTTPSIG_KEY_FILE), note that docker-compose.yaml uses KEYS_DATA_PATH to mount
the host directory so operators can persist keys across restarts, and mention
any interaction with WALLETS_PATH if relevant.
In `@src/app.ts`:
- Around line 49-74: The upload logic must not treat HTTPSIG_ATTESTATION_CACHED
as proof the Arweave upload succeeded; change the condition to run upload when
there is no persisted txId (introduce or use a persistent field like
HTTPSIG_ATTESTATION_TXID or similar) instead of HTTPSIG_ATTESTATION_CACHED, and
ensure setHttpSigAttestationTxId persists the txId to durable storage (not just
in-memory). Update the upload block that calls uploadAttestation(...) and the
catch path accordingly so a transient failure won't prevent future attempts; if
config currently lacks a persistent setter/getter (e.g.,
setHttpSigAttestationTxId, HTTPSIG_ATTESTATION_CACHED), implement a persistent
setter/getter to save the txId to disk/config storage and read it on startup,
and only skip the upload when that persisted txId is present.
In `@src/config.ts`:
- Around line 987-989: The current keysDir calculation uses string slicing with
'/' which fails on Windows; replace that expression to use Node's
path.dirname(HTTPSIG_KEY_FILE) and keep the same fallback to 'data/keys' (i.e.,
const keysDir = path.dirname(HTTPSIG_KEY_FILE) || 'data/keys'); also ensure the
file imports the path module (import path from 'path') at the top if not already
present so path.dirname is available.
---
Nitpick comments:
In `@src/lib/httpsig-upload.ts`:
- Around line 134-142: The arweave.transactions.post call (after
arweave.transactions.sign using tx and jwkAny) currently has no timeout and can
hang startup; add a timeout around the POST: if arweave.transactions.post
supports AbortSignal, create an AbortController, pass controller.signal and call
controller.abort after the timeout; otherwise wrap the post call in a
Promise.race with a timeout-rejecting promise, ensure you cleanly throw a
descriptive error like "Timed out posting attestation L1 transaction" on timeout
and preserve the existing non-200/202 error handling.
In `@src/lib/httpsig.ts`:
- Around line 385-391: The current cache write (built from cacheData and using
mkdirSync(keysDir...) then writeFileSync(cachePath,...)) is not atomic and can
leave a partial file; change it to an atomic write-to-temp-and-rename pattern:
ensure keysDir exists, write the JSON to a temporary file in the same directory
(e.g., cachePath + '.tmp' or a unique temp name), fsync the file (or use
writeFileSync then fsync via the file descriptor) to flush data, close it, then
rename (fs.renameSync) the temp file to cachePath so the replace is atomic;
reference cacheData, keysDir, cachePath, mkdirSync, writeFileSync and perform
the rename step to guarantee atomicity.
In `@src/middleware/httpsig.ts`:
- Around line 47-49: Replace the hardcoded header strings in the middleware's
signature-stripping block with the shared constants: import and use
SIGNATURE_HEADER and SIGNATURE_INPUT_HEADER (the constants defined in
constants.ts) instead of literal 'Signature' and 'Signature-Input'; update the
import statement in src/middleware/httpsig.ts to include those constants and
change the two calls this.removeHeader('Signature') and
this.removeHeader('Signature-Input') to this.removeHeader(SIGNATURE_HEADER) and
this.removeHeader(SIGNATURE_INPUT_HEADER) so the code uses the centralized
header names.
- Around line 109-112: The catch block that currently only calls
metrics.httpSigErrorsTotal.inc() silently swallows signing errors; update that
catch to also log the caught error (without rethrowing) using the module's
logger (e.g., processLogger.warn or logger.debug) so we keep the non-failing
behavior but add observability; target the catch in src/middleware/httpsig.ts
where signing is attempted (the block calling metrics.httpSigErrorsTotal.inc())
and include the error object and contextual text in the log message.
In `@src/routes/ar-io-info-builder.ts`:
- Around line 89-107: Add TSDoc comments for the two new interfaces
HttpsigAttestationInfo and HttpsigInfo similar to the rest of the file: describe
the purpose of HttpsigAttestationInfo (fields txId, observerAddress, payload,
signature, rsaPublicKey and how they represent an HTTPSIG
attestation/verification record) and describe HttpsigInfo (enabled, algorithm,
publicKey, keyId, optional solanaAddress and optional attestation) including the
trust model and when attestation is present; attach the comments immediately
above each interface declaration.
In `@src/routes/ar-io.ts`:
- Around line 131-169: The TSDoc for the /ar-io/info handler is out of date
because the response now includes a new "httpsig" field; update the comment
block above the handler in src/routes/ar-io.ts (the /ar-io/info handler comment
around the existing example JSON) to document the httpsig property (its shape
and when it's present) and add it to the example response JSON so the docs match
the actual response; keep the rest of the existing descriptions (wallet,
processId, bundlers, rateLimiter, x402) unchanged and follow the existing
formatting/style used in the comment.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 857dab8c-61d7-4527-bab5-168a31042737
📒 Files selected for processing (16)
docker-compose.yamldocs/envs.mdpackage.jsonsrc/app.tssrc/config.tssrc/constants.tssrc/lib/httpsig-upload.test.tssrc/lib/httpsig-upload.tssrc/lib/httpsig.test.tssrc/lib/httpsig.tssrc/metrics.tssrc/middleware/httpsig.test.tssrc/middleware/httpsig.tssrc/routes/ar-io-info-builder.tssrc/routes/ar-io.test.tssrc/routes/ar-io.ts
docker-compose.yaml
Outdated
| - ${ETL_STAGING_PATH:-./data/etl/staging}:/app/data/etl/staging | ||
| - ${CDB64_ROOT_TX_INDEX_DATA_PATH:-./data/cdb64-root-tx-index}:/app/data/cdb64-root-tx-index | ||
| - ${KEYS_DATA_PATH:-./data/keys}:/app/data/keys | ||
| - ${WALLETS_PATH:-./wallets}:/app/wallets |
Contributor
There was a problem hiding this comment.
Set WALLETS_PATH to the in-container mount path.
Right now the same variable is used for both the host bind-mount source and the app's in-container lookup path. Any non-default override makes core look for ${WALLETS_PATH}/<wallet>.json on the container filesystem instead of /app/wallets, so observer wallet loading breaks. Keep the host override only in the volume spec, and point WALLETS_PATH at /app/wallets inside the container.
🛠️ Proposed fix
- - ${WALLETS_PATH:-./wallets}:/app/wallets
+ - ${WALLETS_PATH:-./wallets}:/app/wallets:ro
...
- - WALLETS_PATH=${WALLETS_PATH:-}
+ - WALLETS_PATH=/app/walletsAlso applies to: 327-327
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docker-compose.yaml` at line 63, The WALLETS_PATH env is being reused for
both the host bind source and the container lookup which breaks in-container
wallet paths; update the docker-compose service definition so the volume uses
the host override (keep the current ${WALLETS_PATH:-./wallets} as the left side
of the volume mapping) but set the container-side WALLETS_PATH environment value
to the fixed in-container path /app/wallets (i.e., ensure the container ENV
WALLETS_PATH points to /app/wallets while the volume mapping remains
${WALLETS_PATH:-./wallets}:/app/wallets) so code expecting
/app/wallets/<wallet>.json inside the container still works.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #671 +/- ##
===========================================
+ Coverage 77.27% 77.32% +0.04%
===========================================
Files 111 114 +3
Lines 37599 38534 +935
Branches 2803 2843 +40
===========================================
+ Hits 29055 29796 +741
- Misses 8508 8701 +193
- Partials 36 37 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
docker-compose.yaml (1)
62-63: Volume mounts look correct; consider read-only for wallets.The keys and wallets volume mounts are properly configured. The
WALLETS_PATHissue from the previous review is now correctly addressed at line 327.For defense-in-depth, consider mounting wallets read-only like the secrets volume on line 64, since wallet files should only be read at runtime.
🔒 Suggested hardening
- ${KEYS_DATA_PATH:-./data/keys}:/app/data/keys - - ${WALLETS_PATH:-./wallets}:/app/wallets + - ${WALLETS_PATH:-./wallets}:/app/wallets:ro🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docker-compose.yaml` around lines 62 - 63, Make the wallets mount read-only for defense-in-depth: update the docker-compose.yaml service volume that uses ${WALLETS_PATH:-./wallets}:/app/wallets so it mounts read-only (like the secrets volume), leaving the KEYS_DATA_PATH (${KEYS_DATA_PATH:-./data/keys}:/app/data/keys) mount unchanged; ensure the container still has expected read access to /app/wallets and update any startup scripts if they attempt to write there.src/lib/httpsig.ts (1)
264-285: Add TSDoc for the exported attestation contracts.
AttestationPayload,Attestation, andCachedAttestationare part of the module surface, but unlike the surrounding exports they are undocumented. A short TSDoc block clarifying semantics—especially whengatewayAddressis absent and whatrsaPublicKey/txIdrepresent—would make downstream use safer.As per coding guidelines,
**/*.{ts,tsx,js}: Add or improve TSDoc comments when modifying code to enhance documentation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/httpsig.ts` around lines 264 - 285, Add concise TSDoc comments for the exported interfaces AttestationPayload, Attestation, and CachedAttestation: document each field’s semantics (e.g., for AttestationPayload explain that gatewayAddress may be undefined when the gateway is not yet assigned or when attestation is observer-only, clarify issuedAt is ISO8601 timestamp, purpose and type fixed strings), for Attestation describe that payload is the serialized/encoded AttestationPayload, signature is the signer’s signature over payload, and rsaPublicKey is the PEM/DER-encoded RSA public key used for verification; for CachedAttestation note that ed25519PublicKey is the gateway’s ed25519 key and txId is an optional blockchain transaction identifier when the attestation was recorded on-chain. Include brief examples or allowed formats only if helpful and keep comments above the interface declarations AttestationPayload, Attestation, and CachedAttestation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/lib/httpsig.ts`:
- Around line 73-101: The current loadOrGenerateKey can leave a partially
written PEM because it creates the target file before writing; fix by writing
the PEM to a temporary file (e.g., keyFile + '.tmp.' + process.pid or a random
suffix) with mode 0o600, fsync the temp file and its directory, then atomically
rename the temp file into place with renameSync; keep the existing EEXIST
fallback by catching rename errors and loading the existing key via
crypto.createPrivateKey(readFileSync(keyFile,'utf8')) if another process already
installed the file. Ensure these changes are applied inside loadOrGenerateKey
and use the same symbols: crypto.createPrivateKey, writeFileSync (for temp),
fsyncSync, renameSync, and readFileSync.
- Around line 237-248: The containment check in resolveWalletPath is unsafe
because normalize(resolved).startsWith(normalizedBase) can be fooled by sibling
paths; instead compute the path relative to normalizedBase (using path.relative)
and reject if that relative path begins with '..' or is an absolute path,
ensuring resolved is strictly inside walletsPath; update the logic around
resolved/normalizedBase in resolveWalletPath to use this relative-path check and
throw the same error when the check fails.
- Around line 356-369: The cache-load block currently only compares
cached.ed25519PublicKey to currentPubKey and can wrongly reuse an attestation
when the observer wallet or gatewayAddress changed; update the validation in the
cached-attestation loading path (the code that reads cachePath and inspects
CachedAttestation) to also compare the cached gatewayAddress and observer wallet
identifier against the current runtime values (e.g., gatewayAddress and
observerWallet/observerAddress) and only treat it as a hit if all three match;
also extend the CachedAttestation type to store gatewayAddress and observer
wallet identifier so these fields are written when creating/updating the cache
and can be validated on load.
---
Nitpick comments:
In `@docker-compose.yaml`:
- Around line 62-63: Make the wallets mount read-only for defense-in-depth:
update the docker-compose.yaml service volume that uses
${WALLETS_PATH:-./wallets}:/app/wallets so it mounts read-only (like the secrets
volume), leaving the KEYS_DATA_PATH
(${KEYS_DATA_PATH:-./data/keys}:/app/data/keys) mount unchanged; ensure the
container still has expected read access to /app/wallets and update any startup
scripts if they attempt to write there.
In `@src/lib/httpsig.ts`:
- Around line 264-285: Add concise TSDoc comments for the exported interfaces
AttestationPayload, Attestation, and CachedAttestation: document each field’s
semantics (e.g., for AttestationPayload explain that gatewayAddress may be
undefined when the gateway is not yet assigned or when attestation is
observer-only, clarify issuedAt is ISO8601 timestamp, purpose and type fixed
strings), for Attestation describe that payload is the serialized/encoded
AttestationPayload, signature is the signer’s signature over payload, and
rsaPublicKey is the PEM/DER-encoded RSA public key used for verification; for
CachedAttestation note that ed25519PublicKey is the gateway’s ed25519 key and
txId is an optional blockchain transaction identifier when the attestation was
recorded on-chain. Include brief examples or allowed formats only if helpful and
keep comments above the interface declarations AttestationPayload, Attestation,
and CachedAttestation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 854d9ea3-bd94-48ec-8f2f-464e27cca557
📒 Files selected for processing (8)
docker-compose.yamldocs/envs.mdsrc/app.tssrc/config.tssrc/lib/httpsig.test.tssrc/lib/httpsig.tssrc/middleware/httpsig.tssrc/routes/ar-io-info-builder.ts
✅ Files skipped from review due to trivial changes (2)
- docs/envs.md
- src/lib/httpsig.test.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- src/middleware/httpsig.ts
- src/routes/ar-io-info-builder.ts
- src/config.ts
- src/app.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/openapi.yaml`:
- Around line 1092-1101: The 206 response schema is missing the RFC 9421
signature headers even though the httpsig middleware (src/middleware/httpsig.ts)
signs any response with trust-relevant headers; update the 206 response in the
OpenAPI spec to include the same Signature and Signature-Input header
definitions (with type string, description, and examples) as used for the 200
response so partial content responses show the documented Signature and
Signature-Input headers.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/lib/httpsig.ts`:
- Around line 83-99: The current atomic write uses renameSync(tmpFile, keyFile)
which can overwrite an existing destination; change this to linkSync(tmpFile,
keyFile) inside the same try block (around the openSync/writeFileSync/closeSync
sequence for tmpFile) so the operation fails with EEXIST when another process
already installed the key, and update the surrounding catch handler that
currently catches err to specifically handle EEXIST by loading the
already-installed keyFile (instead of treating it as a generic write error);
keep the tmp file cleanup logic as-is so unsuccessful writers remove their
tmpFile.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: d1d888f8-0328-417a-828f-03c54ef419c8
📒 Files selected for processing (3)
docker-compose.yamldocs/openapi.yamlsrc/lib/httpsig.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/openapi.yaml
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/lib/httpsig.ts`:
- Around line 427-440: The temp file path used for atomic caching (tmpPath) can
collide across processes; update the code in the attestation cache routine
(where cacheData is built and written using writeFileSync and renameSync, and
keysDir/cachePath/tmpPath are used) to create a unique temp filename per
invocation (e.g., append process.pid plus a high-entropy suffix like Date.now()
or a UUID) instead of a fixed `${cachePath}.tmp`; write to that unique tmp file
and then renameSync it into cachePath, optionally catching and ignoring ENOENT
on rename to avoid startup crashes from benign races.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
Comment on lines
+427
to
+440
| // Cache to disk atomically (write to temp, then rename) | ||
| const cacheData: CachedAttestation = { | ||
| ...attestation, | ||
| ed25519PublicKey: currentPubKey, | ||
| observerAddress: currentObserverAddr, | ||
| gatewayAddress, | ||
| }; | ||
| mkdirSync(keysDir, { recursive: true }); | ||
| const tmpPath = `${cachePath}.tmp`; | ||
| writeFileSync(tmpPath, JSON.stringify(cacheData, null, 2)); | ||
| renameSync(tmpPath, cachePath); | ||
|
|
||
| return { ...attestation, cached: false }; | ||
| } |
Contributor
There was a problem hiding this comment.
Add unique suffix to temp file path to prevent startup race failures.
The shared tmpPath (line 435) can cause ENOENT if two processes race during attestation creation: Process A writes, Process B writes and renames, Process A's rename fails because the temp file no longer exists.
While both attestations would be valid (differing only in issuedAt), the losing process throws an unhandled error that fails startup.
🛠️ Suggested fix
mkdirSync(keysDir, { recursive: true });
- const tmpPath = `${cachePath}.tmp`;
+ const tmpPath = `${cachePath}.tmp.${process.pid}`;
writeFileSync(tmpPath, JSON.stringify(cacheData, null, 2));
- renameSync(tmpPath, cachePath);
+ try {
+ renameSync(tmpPath, cachePath);
+ } finally {
+ try {
+ unlinkSync(tmpPath);
+ } catch {
+ // Ignore - temp file may have been renamed or already cleaned
+ }
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/httpsig.ts` around lines 427 - 440, The temp file path used for
atomic caching (tmpPath) can collide across processes; update the code in the
attestation cache routine (where cacheData is built and written using
writeFileSync and renameSync, and keysDir/cachePath/tmpPath are used) to create
a unique temp filename per invocation (e.g., append process.pid plus a
high-entropy suffix like Date.now() or a UUID) instead of a fixed
`${cachePath}.tmp`; write to that unique tmp file and then renameSync it into
cachePath, optionally catching and ignoring ENOENT on rename to avoid startup
crashes from benign races.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
HTTPSIG_ENABLED=true)Signature+Signature-Inputheaders signed by an Ed25519 key (~33us per request)keyIdembeds full public key — no lookup needed for verificationTest plan
HTTPSIG_ENABLED=false(default) has zero impact on existing behavior