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

fix: throw error on invalid URLs when setting cookie #18756

Merged
merged 6 commits into from Jun 14, 2019

Conversation

@erickzhao
Copy link
Member

commented Jun 12, 2019

Description of Change

With this PR, invalid inputs to the url parameter will throw an error when using cookie.set(). This is done by checking if the URL is parseable using GURL rather than checking if the URL string being passed in is empty.

Previously, invalid URLs would be able to be added as a cookie, but you would not be able to filter for them or remove them.

See Electron Fiddle gist: https://gist.github.com/d8ae35040d44d426cb40d97d5d77a47e

N.B. This behavior was fixed in Electron 6, so we based this PR a lot on the changes in the cookies API from 5.0.3 to 6.0.0-beta.

This issue was discovered when playing around with the example from #18220.

cc @DeerMichel @codebytere @ckerr

Checklist

Release Notes

Notes: Added check for invalid URLs upon setting cookies

@erickzhao erickzhao requested a review from codebytere Jun 12, 2019

@welcome

This comment has been minimized.

Copy link

commented Jun 12, 2019

💖 Thanks for opening this pull request! 💖

We use semantic commit messages to streamline the release process. Before your pull request can be merged, you should update your pull request title to start with a semantic prefix.

Examples of commit messages with semantic prefixes:

  • fix: don't overwrite prevent_default if default wasn't prevented
  • feat: add app.isPackaged() method
  • docs: app.isDefaultProtocolClient is now available on Linux

Things that will help get your PR across the finish line:

  • Follow the JavaScript, C++, and Python coding style.
  • Run npm run lint locally to catch formatting errors earlier.
  • Document any user-facing changes you've made following the documentation styleguide.
  • Include tests when adding/changing behavior.
  • Include screenshots and animated GIFs whenever possible.

We get a lot of pull requests on this repo, so please be patient and we will get back to you as soon as we can.

@erickzhao erickzhao changed the title fix: Throw error on invalid URLs on cookie creation fix: Throw error on invalid URLs when setting cookie Jun 12, 2019

@erickzhao erickzhao changed the title fix: Throw error on invalid URLs when setting cookie fix: throw error on invalid URLs when setting cookie Jun 12, 2019

@erickzhao erickzhao marked this pull request as ready for review Jun 12, 2019

@ckerr ckerr added the semver/minor label Jun 12, 2019

@deepak1556
Copy link
Member

left a comment

LGTM with minor change. Thanks!

Show resolved Hide resolved atom/browser/api/atom_api_cookies.cc Outdated
Show resolved Hide resolved docs/api/cookies.md Outdated
Show resolved Hide resolved atom/browser/api/atom_api_cookies.cc Outdated
fix: is_empty -> is_valid
Co-Authored-By: Samuel Attard <samuel.r.attard@gmail.com>

DeerMichel added a commit that referenced this pull request Jun 13, 2019

DeerMichel added a commit that referenced this pull request Jun 13, 2019

@DeerMichel DeerMichel referenced this pull request Jun 13, 2019

Merged

fix: use is_valid for cookie url validation #18770

4 of 4 tasks complete
Update docs/api/cookies.md
Co-Authored-By: Samuel Attard <samuel.r.attard@gmail.com>
@MarshallOfSound
Copy link
Member

left a comment

Temporarily requesting a hold while we figure out why the is_valid change blew up the test suite on master --> #18770 (review)

@codebytere codebytere merged commit b034bf9 into 5-0-x Jun 14, 2019

11 of 14 checks passed

Valid Backport Invalid Backport
Details
electron-arm-testing Build #20190613.27 had test failures
Details
Backportable? - 4-2-x Backport Failed
Details
Artifact Comparison No Changes
Details
Semantic Pull Request ready to be squashed
Details
WIP Ready for review
Details
appveyor: win-ia32-testing AppVeyor build succeeded
Details
appveyor: win-ia32-testing-pr AppVeyor build succeeded
Details
appveyor: win-x64-testing AppVeyor build succeeded
Details
appveyor: win-x64-testing-pr AppVeyor build succeeded
Details
build-linux Workflow: build-linux
Details
build-mac Workflow: build-mac
Details
electron-arm64-testing Build #20190613.28 succeeded
Details
release-notes Release notes found
@release-clerk

This comment has been minimized.

Copy link

commented Jun 14, 2019

Release Notes Persisted

Added check for invalid URLs upon setting cookies

@welcome

This comment has been minimized.

Copy link

commented Jun 14, 2019

Congrats on merging your first pull request! 🎉🎉🎉

@codebytere codebytere deleted the intern/throw-error-invalid-cookie-url branch Jun 14, 2019

@trop

This comment has been minimized.

Copy link
Contributor

commented Jun 14, 2019

I was unable to backport this PR to "4-2-x" cleanly;
you will need to perform this backport manually.

@trop trop bot added needs-manual-bp/4-2-x and removed target/4-2-x labels Jun 14, 2019

@codebytere

This comment has been minimized.

Copy link
Member

commented Jun 14, 2019

@erickzhao trop failed us here; can you open a manual bp to 4-2-x?

@erickzhao erickzhao referenced this pull request Jun 14, 2019

Closed

Cookie support broken in Electron 5.0.x? #18220

3 of 3 tasks complete

@sofianguy sofianguy added this to Fixed in 5.0.4 in 5.0.x Jun 18, 2019

@erickzhao erickzhao referenced this pull request Jun 20, 2019

Closed

fix: throw error on invalid URLs when setting cookie #18911

5 of 5 tasks complete
@trop

This comment has been minimized.

Copy link
Contributor

commented Jun 20, 2019

A maintainer has manually backported this PR to "4-2-x", please check out #18911

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.