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

@erickzhao erickzhao 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

@electron-cation electron-cation bot added the new-pr 🌱 PR opened in the last 24 hours label Jun 12, 2019
@welcome
Copy link

welcome bot 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 June 12, 2019 22:30
@ckerr ckerr added the semver/minor backwards-compatible functionality label Jun 12, 2019
Copy link
Member

@deepak1556 deepak1556 left a comment

Choose a reason for hiding this comment

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

LGTM with minor change. Thanks!

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 Show resolved Hide resolved
Co-Authored-By: Samuel Attard <samuel.r.attard@gmail.com>
@codebytere codebytere added backport This is a backport PR target/4-2-x and removed new-pr 🌱 PR opened in the last 24 hours target/5-0-x labels Jun 13, 2019
deermichel pushed a commit that referenced this pull request Jun 13, 2019
deermichel pushed a commit that referenced this pull request Jun 13, 2019
Co-Authored-By: Samuel Attard <samuel.r.attard@gmail.com>
Copy link
Member

@MarshallOfSound MarshallOfSound left a comment

Choose a reason for hiding this comment

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

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
@release-clerk
Copy link

release-clerk bot commented Jun 14, 2019

Release Notes Persisted

Added check for invalid URLs upon setting cookies

@welcome
Copy link

welcome bot commented Jun 14, 2019

Congrats on merging your first pull request! 🎉🎉🎉

@codebytere codebytere deleted the intern/throw-error-invalid-cookie-url branch June 14, 2019 17:54
@trop
Copy link
Contributor

trop bot commented Jun 14, 2019

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

@codebytere
Copy link
Member

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

@trop
Copy link
Contributor

trop bot 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
Labels
backport This is a backport PR semver/minor backwards-compatible functionality
Projects
No open projects
5.0.x
Fixed in 5.0.4
Development

Successfully merging this pull request may close these issues.

None yet

6 participants