CLDSRV-881: fix missing CORS headers on 403 responses#6126
CLDSRV-881: fix missing CORS headers on 403 responses#6126bert-e merged 2 commits intodevelopment/9.2from
Conversation
Hello tcarmet,My role is to assist you with the merge of this Available options
Available commands
Status report is not available. |
Incorrect fix versionThe
Considering where you are trying to merge, I ignored possible hotfix versions and I expected to find:
Please check the |
|
LGTM |
Codecov Report❌ Patch coverage is
Additional details and impacted files
... and 3 files with indirect coverage changes @@ Coverage Diff @@
## development/9.2 #6126 +/- ##
===================================================
+ Coverage 84.36% 84.50% +0.14%
===================================================
Files 204 204
Lines 13159 13180 +21
===================================================
+ Hits 11101 11138 +37
+ Misses 2058 2042 -16
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
tcarmet
force-pushed
the
bugfix/CLDSRV-881-cors-headers-on-403-responses
branch
from
d7eec24 to
f073231
Compare
Incorrect fix versionThe
Considering where you are trying to merge, I ignored possible hotfix versions and I expected to find:
Please check the |
|
LGTM |
tcarmet
force-pushed
the
bugfix/CLDSRV-881-cors-headers-on-403-responses
branch
from
f073231 to
66233f9
Compare
|
LGTM |
tcarmet
force-pushed
the
bugfix/CLDSRV-881-cors-headers-on-403-responses
branch
from
66233f9 to
196a998
Compare
Incorrect fix versionThe
Considering where you are trying to merge, I ignored possible hotfix versions and I expected to find:
Please check the |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
LGTM — the callback-wrapping approach in api.js is clean: it only pays the extra metadata lookup on error+Origin, guards headersSent, and correctly short-circuits on success. Two minor test quality notes posted inline. |
tcarmet
force-pushed
the
bugfix/CLDSRV-881-cors-headers-on-403-responses
branch
3 times, most recently
from
406c33e to
bf9577a
Compare
Incorrect fix versionThe
Considering where you are trying to merge, I ignored possible hotfix versions and I expected to find:
Please check the |
|
Clean, well-structured fix. The wrapper approach correctly limits the extra metadata lookup to error + Origin scenarios, and the fast-path check via hasCorsHeaders avoids redundant lookups when handlers already computed CORS headers. |
tcarmet
force-pushed
the
bugfix/CLDSRV-881-cors-headers-on-403-responses
branch
from
5d9b95f to
5ce3f67
Compare
|
LGTM — clean approach wrapping the callback at the |
|
LGTM |
|
LGTM |
| // We only pay the extra metadata lookup when an Origin header is present, | ||
| // matching AWS behavior and the existing contract of collectCorsHeaders. | ||
| function wrapCallbackWithErrorCorsHeaders(request, response, log, callback) { | ||
| return (err, ...rest) => { |
There was a problem hiding this comment.
If the handler computes corsHeaders and gets back {} (bucket has no CORS config), and then calls callback(err, null, {}), the wrapper will still do metadata.getBucket, right? (but it does not need to, I think).
Not sure if it's a good idea, but we could cache the bucket in request (something like request._loadedBucket) and first check there before metadata.getBucket
There was a problem hiding this comment.
Was able to confirm this with a extra test, indeed there would be 2 metadata call in this case.
So yeah we could consider {} as there's no header, and skip the bucket call in this case, it seems to be implicitly true, or we cache the bucket as you stated.
I'm debating between both right now, I pushed a code with the bucket cache on request. seems like the safest option between the two. Let me know what you think.
There was a problem hiding this comment.
Something I didn't consider when I suggested the cache approach: I think objectCopy and objectPutCopyPart call standardMetadataValidateBucketAndObj twice (once for the destination, once for the source), so the second call will overwrite the first, and we may not get the headers for the bucket we want.
You could confirm with a test.
There was a problem hiding this comment.
yes confirmed with the tests, nice catch again. At the end I ended up droping that cache entirely and accepting up to 2 metadata.getBucket calls on the error path. I believe the trade-off is fine here because:
- This only fires on failing requests with an Origin header where the handler returned without surfacing CORS headers (no rule match, no CORS config, or handler dropped the bucket between waterfall steps. Small slice of overall traffic.
- Turns out going the other way and trusting
{}as "no CORS to apply" would have regressed handlers that drop the bucket internally their collectCorsHeaders runs with bucket=undefined and returns{}even when a matching CORS rule exists. We'd silently lose CORS headers we currently apply. - A smarter cache (map keyed by
bucketNamesoobjectCopydoesn't overwrite the destination with the source) would have preserved the optimization without the leak you flagged. So that is an option we could arguably go this way, but I honestly felt like it was a over-optimization for an error-path issue, so I went with the simpler code.
To sum things up I removed the cache, documented the behavior in case we decide to optimize in the future and added/adapted the test cases (objectCopy and limiting number of calls to getBucket) so that if we do decide to mess with it, we ensure what we discussed is not lost and will fail if we handle it badly.
There was a problem hiding this comment.
That's good with me. Thanks for taking the time to look at the optimization.
|
LGTM |
|
LGTM |
| // Callback arity varies per route ((err, corsHeaders), (err, xml, | ||
| // corsHeaders), (err, dataGetInfo, resMetaHeaders, range), ...) so we | ||
| // scan the args instead of relying on a fixed position. | ||
| function hasCorsHeaders(rest) { |
There was a problem hiding this comment.
nitpick: not fond of rest as a name, what about callbackArgs.
Also wondering if there's a reason to check every argument, i.e. if the CORS headers shouldn't always be returned as the third callback argument as a convention?
There was a problem hiding this comment.
There's no clear separation of the CORS headers, it's not always returned as the third callback, it's mix of the second or third depending on the routing it takes. (I added a comment already stating, maybe it wasn't clear. let me know)
Tackling a standard position would be a refactor of it's own IMHO not in scope right now.
|
LGTM |
Incorrect fix versionThe
Considering where you are trying to merge, I ignored possible hotfix versions and I expected to find:
Please check the |
Cross-origin browser clients lost Access-Control-* headers whenever cloudserver returned 403, even though the bucket had a matching CORS rule. Responses now include the CORS headers for error responses on buckets with CORS configured, matching AWS behavior.
tcarmet
force-pushed
the
bugfix/CLDSRV-881-cors-headers-on-403-responses
branch
from
1ab24f2 to
3adf9ff
Compare
Integration data createdI have created the integration data for the additional destination branches.
The following branches will NOT be impacted:
You can set option The following options are set: approve |
|
LGTM |
|
LGTM |
Build failedThe build for commit did not succeed in branch w/9.3/bugfix/CLDSRV-881-cors-headers-on-403-responses The following options are set: approve |
Request integration branchesWaiting for integration branch creation to be requested by the user. To request integration branches, please comment on this pull request with the following command: Alternatively, the |
|
/approve |
Integration data createdI have created the integration data for the additional destination branches.
The following branches will NOT be impacted:
You can set option The following options are set: approve |
Build failedThe build for commit did not succeed in branch w/9.4/bugfix/CLDSRV-881-cors-headers-on-403-responses The following options are set: approve |
|
I have successfully merged the changeset of this pull request
The following branches have NOT changed:
Please check the status of the associated issue CLDSRV-881. Goodbye tcarmet. The following options are set: approve |
4 participants
When a bucket has CORS configured, cloudserver was omitting
Access-Control-*headers from 403 responses. Browsers treat such responses as opaque and block the error from the web app, even though AWS S3 returns CORS headers on error responses when a matching rule exists.The existing
collectCorsHeadershelper only runs inside an API handler, after bucket metadata has been loaded. Auth failures from Vault return before any handler runs, so the route-level error response path sent no CORS headers.The fix wraps
callApiMethod's callback: on error, when the request has anOriginheader and a known bucket, it fetches the bucket's CORS config and sets the matchingAccess-Control-*headers directly on the HTTP response before propagating the error. The extra metadata lookup only happens on failed requests with anOriginheader - no cost for non-CORS traffic or successful requests.Fixes: CLDSRV-881