Skip to content
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.

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

[WIP] Query Params #7664

Closed
wants to merge 4 commits into from
Closed

[WIP] Query Params #7664

wants to merge 4 commits into from

Conversation

sfc-gh-wihuang
Copy link
Contributor

Describe your changes

GitHub Issue Link (if applicable)

Testing Plan

  • Explanation of why no additional tests are needed
  • Unit Tests (JS and/or Python)
  • E2E Tests
  • Any manual testing needed?

Contribution License Agreement

By submitting this pull request you agree that all contributions to this project are made under the Apache 2.0 license.

willhuang1997 and others added 4 commits October 27, 2023 17:19
…QueryParam class (#7621)

* Add clear, clear, __delitem__, __len__, __contains__, __init__ methods with tests


---------

Co-authored-by: willhuang1997 <willhuang1997@gmail.com>
* Remove unnecessary method as I realize the internal state of query params will not need to use this method

* Fix e2e test

* Fix python tests
@vdonato
Copy link
Collaborator

vdonato commented Nov 4, 2023

I initially thought this wouldn't require any security process since we're not changing anything about how query params are sent over the wire, but given that #7619 would have introduced a vulnerability where users would be able to see query params set by other sessions, we should probably go through the full process for this to be safe.

@sfc-gh-wihuang
Copy link
Contributor Author

Closing this as I would like to create a new branch as we have come to the conclusion that we likely don't want a separate QueryParams class. The new branch will be feature/st_query_params_2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants