fix: route signed Harness log blobs through client#199
Conversation
Co-authored-by: Rohan Gupta <thisrohangupta@users.noreply.github.com>
Co-authored-by: Rohan Gupta <thisrohangupta@users.noreply.github.com>
Co-authored-by: Rohan Gupta <thisrohangupta@users.noreply.github.com>
Co-authored-by: Rohan Gupta <thisrohangupta@users.noreply.github.com>
Co-authored-by: Rohan Gupta <thisrohangupta@users.noreply.github.com>
Co-authored-by: Rohan Gupta <thisrohangupta@users.noreply.github.com>
|
|
Co-authored-by: Rohan Gupta <thisrohangupta@users.noreply.github.com>
There was a problem hiding this comment.
- Important: relative signed blob links are still rewritten as unsigned paths. The new
isSignedBlobUrlcheck only runs whensafeParseUrl(blobLink)succeeds, so links like/some/blob/path?X-Amz-Signature=sigorsome/blob/path?...still fall through the unsigned branch. With the realHarnessClient, that becomes.../gateway/log-service/some/blob/path?accountIdentifier=...&routingId=...&accountID=...&X-Amz-Signature=sig, which mutates the signed URL and can recreate the download failure this PR is trying to fix. Detect signed relative links too (for example by parsing with a dummy base or inspecting the raw query string before URL parsing) and add a regression test for that shape.
Sent by Cursor Automation: Sunil On Demand Architecture Review
| signal: AbortSignal, | ||
| ): Promise<Response> { | ||
| const blobUrl = safeParseUrl(blobLink); | ||
| const isSignedBlobUrl = blobUrl ? isPresignedUrl(blobUrl) : false; |
There was a problem hiding this comment.
Relative signed links are still misclassified here. safeParseUrl() returns undefined for values like /some/blob/path?X-Amz-Signature=sig, so isSignedBlobUrl becomes false and the code later prepends /gateway/log-service and omits headerBasedScoping. Running that through the real HarnessClient produces a URL like https://<base>/gateway/log-service/some/blob/path?accountIdentifier=...&routingId=...&accountID=...&X-Amz-Signature=sig, which mutates the signed blob URL. Please detect signed relative paths too and add a regression test for that case.
There was a problem hiding this comment.
Found 1 important issue before this is architecture-safe to merge:
src/utils/log-resolver.ts: relative signed blob links are still treated as unsigned. The new logic only checksisPresignedUrl()whennew URL(blobLink)succeeds, but relative links likesome/blob/path?X-Amz-Signature=sig&token=abcdo not parse without a base. That meansdownloadPathis rewritten through/gateway/log-service, andHarnessClient.buildUrl()appendsaccountIdentifier,routingId, andaccountID, mutating the signed query string that this PR is trying to preserve.
Assumption:
I am assuming the log-service can return relative signed links as well as absolute ones. If that invariant is impossible, please document it in code/tests; otherwise this needs a regression test and fix.
Verification:
pnpm test tests/utils/log-resolver.test.tspnpm typecheckpnpm build- direct repro after build:
some/blob/path?X-Amz-Signature=sig&token=abcbecamehttps://app.harness.io/gateway/log-service/some/blob/path?accountIdentifier=acct&routingId=acct&accountID=acct&X-Amz-Signature=sig&token=abc
Sent by Cursor Automation: Sunil On Demand Architecture Review
| signal: AbortSignal, | ||
| ): Promise<Response> { | ||
| const blobUrl = safeParseUrl(blobLink); | ||
| const isSignedBlobUrl = blobUrl ? isPresignedUrl(blobUrl) : false; |
There was a problem hiding this comment.
This still misses relative signed links. new URL(blobLink) returns undefined for inputs like some/blob/path?X-Amz-Signature=sig&token=abc, so isSignedBlobUrl becomes false here and the code falls into the /gateway/log-service rewrite below. With the real HarnessClient, that rewritten path then picks up accountIdentifier, routingId, and accountID, which mutates the signed query string and breaks the blob download. Please parse relative links against a dummy base or inspect the raw query string before deciding whether to preserve the path and enable headerBasedScoping.


Description
Fixes log blob download routing for Harness-hosted signed URLs. Links on Harness hosts that include
X-Amz-SignatureorX-Goog-Signaturenow route throughHarnessClient.requestStream()instead of directfetch(), preserving proxy/auth routing for self-managed and proxied deployments.The resolver now direct-fetches only recognized external storage hosts (S3/GCS), preserves signed non-storage path/query values with header-based scoping, normalizes relative blob paths before adding
/gateway/log-service, preservesHarnessApiErrordetails, and keeps path-style regional S3 URLs direct-fetchable.Type of Change
Checklist