CLDSRV-881: fix missing CORS headers on 403 responses#6126
CLDSRV-881: fix missing CORS headers on 403 responses#6126tcarmet wants to merge 7 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 1 file with indirect coverage changes @@ Coverage Diff @@
## development/9.2 #6126 +/- ##
===================================================
+ Coverage 84.29% 84.36% +0.06%
===================================================
Files 204 204
Lines 13151 13181 +30
===================================================
+ Hits 11086 11120 +34
+ Misses 2065 2061 -4
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
2 times, most recently
from
44b8aa0 to
406c33e
Compare
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
406c33e to
bf9577a
Compare
| || !request.bucketName) { | ||
| return callback(err, ...rest); | ||
| } | ||
| return metadata.getBucket(request.bucketName, log, (mdErr, bucket) => { |
There was a problem hiding this comment.
Are there any cases in which api handlers pass corsHeaders to the callback on error (when they have already done the bucket lookup), and we can use them to avoid the getBucket ?
There was a problem hiding this comment.
There are, good catch. The only annoying part is that the parameter placement in which they can be returned is a bit inconsistent but I've added a a little helper to help us identify the cases where there can be one. Also added a extra testcase for it.
There was a problem hiding this comment.
Also your suggestion found a couple of pre-existing bug on bucketGetCors and bucketGetWebsite so nice 👌
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 |
bucketGetCors and bucketGetWebsite were passing corsHeaders at callback position 1 on AccessDenied, but both endpoints are routed through responseXMLBody which reads corsHeaders at position 2. The headers were silently dropped by the route layer, so 403 responses arrived without them.
|
LGTM |
| return callback(err, corsHeaders); | ||
| } | ||
| return callback(err); | ||
| return callback(err, null, corsHeaders); |
There was a problem hiding this comment.
The same issue seems to exist here:
cloudserver/lib/api/bucketGetLocation.js
Line 33 in bb5ca88
There was a problem hiding this comment.
good catch, thanks. There are other calls where corsHeaders is in the same position but on those cases the routing is correct. I think this should be the last one in the wrong position.
| // 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.
bucketGetLocation had the same AccessDenied arity mismatch as bucketGetCors and bucketGetWebsite: the error branch passed corsHeaders at callback position 1, but the GET route reads position 2. Align it with the standard three-argument error callback used elsewhere.
When a handler has already loaded the bucket, the CORS error wrapper no longer re-fetches it to compute headers.
|
LGTM |
3 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