fix(storage): pass signingEndpoint to URLSigner in file.getSignedUrl - #8982
fix(storage): pass signingEndpoint to URLSigner in file.getSignedUrl#8982thiyaguk09 wants to merge 3 commits into
Conversation
There was a problem hiding this comment.
Code Review
This pull request adds support for passing a signingEndpoint configuration option to URLSigner when generating a signed URL, along with corresponding unit tests. The feedback suggests spreading the base configuration (SIGNED_URL_CONFIG) before applying the signingEndpoint property in the test to prevent accidental overwrites.
5b15d39 to
4074d75
Compare
4074d75 to
9599274
Compare
| contentMd5: cfg.contentMd5, | ||
| contentType: cfg.contentType, | ||
| host: cfg.host, | ||
| signingEndpoint: cfg.signingEndpoint, |
There was a problem hiding this comment.
When URLSigner receives this parameter, it passes it to auth.sign() as a second argument. The issue is that the public AuthClient interface only defines one parameter (sign(blobToSign: string)).
If a developer provides a custom AuthClient implementation based on our interface, Javascript will silently drop this new signingEndpoint argument at runtime. The user will receive a signed URL generated from the default endpoint, and they'll have no idea their custom config was ignored.
Suggestion: Could we update the AuthClient interface in src/signer.ts to explicitly include the optional parameter? sign(blobToSign: string, signingEndpoint?: string): Promise;
This will ensure custom implementers are aware they need to support it.
There was a problem hiding this comment.
I've updated the AuthClient interface in src/signer.ts, it is now sign(blobToSign: string, signingEndpoint?: string): Promise<string>;
rajeevpodar
left a comment
There was a problem hiding this comment.
Verify the custom auth support code flow.
9599274 to
63b2ee2
Compare
4b75dd8 to
b528a85
Compare
b528a85 to
bd98c61
Compare
Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:
Fixes #8829