Skip to content

fix: validate action of getSignedUrl() function #684

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
AVaksman merged 3 commits into from
May 1, 2019
Merged

fix: validate action of getSignedUrl() function #684

AVaksman merged 3 commits into from
May 1, 2019

Conversation

@AVaksman
Copy link
Contributor

@AVaksman AVaksman commented Apr 30, 2019 •
edited
Loading

Fixes #671

  • Tests and linter pass
  • Code coverage does not decrease

add validation for action parameter.

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Apr 30, 2019
@AVaksman AVaksman changed the title validate action of getSignedUrl() function fix: validate action of getSignedUrl() function Apr 30, 2019
@codecov
Copy link

codecov bot commented Apr 30, 2019 •
edited
Loading

Codecov Report

Merging #684 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #684      +/-   ##
==========================================
+ Coverage   98.04%   98.04%   +<.01%     
==========================================
  Files           9        9              
  Lines         921      922       +1     
  Branches      100      100              
==========================================
+ Hits          903      904       +1     
  Misses          9        9              
  Partials        9        9
Impacted Files Coverage Δ
src/file.ts 98.8% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8b40e6a...7d27223. Read the comment docs.

bcoe
bcoe approved these changes Apr 30, 2019
View reviewed changes
Copy link

@bcoe bcoe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What was the behavior of the old API, if I were to pass in an invalid action? This looks great, my only concern is it might be a breaking change:

  • if the old logic would have failed with a more cryptic error, this seems reasonable as a non-breaking change.
  • if prior to this we would have succeeded, this feels like a breaking change.

Assuming we would have previously been throwing an exception, I'm 👍 on this change.

src/file.ts Outdated
*/
getSignedUrl(cfg: GetSignedUrlConfig, callback?: GetSignedUrlCallback):
void|Promise<GetSignedUrlResponse> {
const validAction = ['read', 'write', 'delete', 'resumable'];
Copy link

@bcoe bcoe Apr 30, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks reasonable to me, might be worth considering using an Enum for TypeScript, does this translate cleanly to the JavaScript API?

Copy link
Contributor

@stephenplusplus stephenplusplus Apr 30, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's actually already this enum:

nodejs-storage/src/file.ts

Line 2341 in 7b1ef2c

const method = ActionToHTTPMethod[cfg.action];

We could just validate that method is something.

Copy link
Contributor Author

@AVaksman AVaksman Apr 30, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, done

@AVaksman AVaksman marked this pull request as ready for review April 30, 2019 21:12
@AVaksman
Copy link
Contributor Author

AVaksman commented Apr 30, 2019

What was the behavior of the old API, if I were to pass in an invalid action? This looks great, my only concern is it might be a breaking change:

  • if the old logic would have failed with a more cryptic error, this seems reasonable as a non-breaking change.
  • if prior to this we would have succeeded, this feels like a breaking change.

Assuming we would have previously been throwing an exception, I'm 👍 on this change.

Previously getSignedUrl with invalid action parameter would still successfully return an invalid URL (a URL with permissions for non existing action so to speak) and any request via this URL would result with SignatureDoesNotMatch response. The error is cryptic since it points user in completely different direction.

@bcoe
Copy link

bcoe commented Apr 30, 2019

@AVaksman feels like a fix to me, I continue to be a 👍

@AVaksman AVaksman merged commit 1b09d24 into googleapis:master May 1, 2019
@AVaksman AVaksman deleted the validate_action branch May 13, 2019 20:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers

@stephenplusplus stephenplusplus stephenplusplus left review comments

@bcoe bcoe bcoe approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

cla: yes This human has signed the Contributor License Agreement.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

getSignedUrl fails silently with invalid url if action is invalid/undefined

4 participants

@AVaksman @bcoe @stephenplusplus @googlebot