Skip to content
This repository was archived by the owner on Apr 6, 2022. It is now read-only.

Conversation

patrickvenetz
Copy link
Contributor

@patrickvenetz patrickvenetz commented May 7, 2018

Ensures email query parameter in verification URL is encoded as base64 in an url-safe manner and ensures emailFromQuery in Session::sessionByToken() is a plain string.

URLs containin email=<email> or email=<urlencoded email> instead of email=<base64-encoded email> let to double-encoding conflicts in some email clients, and resulted in URLs being invalid:

heidi@wiese.tld is encoded to heidi%40wiese.tld (@ => %40). Some email clients would re-encode and generate a link with heidi%2540wiese.tld (% => %25).

⚠️ This is a breaking change and requires orbiting/republik-frontend#101 to be merged an deployed first. Deployed.

Introduces urlsafe-base64 as a dependency.

@patrickvenetz patrickvenetz requested a review from patte May 7, 2018 19:40
@coveralls
Copy link

coveralls commented May 7, 2018

Pull Request Test Coverage Report for Build 233

  • 0 of 0 (NaN%) changed or added relevant lines in 0 files are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage remained the same at 44.062%

Files with Coverage Reduction New Missed Lines %
server.js 1 117.02%
Totals Coverage Status
Change from base Build 227: 0.0%
Covered Lines: 1859
Relevant Lines: 3547

💛 - Coveralls

@patte
Copy link
Contributor

patte commented May 8, 2018

this looks beautiful: https://github.com/orbiting/republik-frontend/pull/101/files
Not just because this is auth related "lava-ground" I think both the FE and BE implementation should be the same. I see two options, copying lib/utils/base64u.js and care about idempotent orbiting npm packages later, or invest in them now. What do you think? Other ideas?

@patrickvenetz
Copy link
Contributor Author

@patte Thought about it a bit more and find base64u not to be in a good, healthy, stable state yet to be published.

I do not fully comprehent what these implemented work-around methods do.

But fair enough: It is not likely to change much overtime as it has a single purpose, does that one for us pretty neatly.

I see two options, copying lib/utils/base64u.js and care about idempotent orbiting npm packages later, or invest in them now.

Hence going for first option.

Bäm.

@patrickvenetz patrickvenetz force-pushed the encode-emails-urlsafe branch from 1610317 to 90f5239 Compare May 9, 2018 09:27
@patrickvenetz
Copy link
Contributor Author

@patte Please revisit.

@patte
Copy link
Contributor

patte commented May 9, 2018

@patrickvenetz please check the failing travis tests:
Error: Cannot find module '/home/travis/build/orbiting/backends/servers/publikator/?(__tests__|lib|graphql)/**/*.test.js'

It does conflict with servers/republik. Test setup in servers/republik
does some magic node_modules symlinking.

When faucet is installed, it comes with its own tape binary.

faucet/tape however is not compatible with tape/tape.
@patrickvenetz
Copy link
Contributor Author

Did revert installation of faucet as a dependency in base64u package. Failing TravisCI checks did recover.

faucet comes with ancient a tape dependency.

A combination of yarn's resolution, linking of packages and servers/republik script test:prepare is removing and symlinking a node_module folder](https://github.com/orbiting/backends/blob/encode-emails-urlsafe/servers/republik/package.json#L56) leads to have .bin/tape to be not a recent version but the faucet's ancient one.

That ancient one does not support globbing (e.g. **/*.js) which let to failing TravisCI tests.

💥

Good news: This can be straightened, with love and dedication (declaring packages, removing outside-the-box-attempts), but would like to not do so in this Merge Request.

@patrickvenetz patrickvenetz merged commit 298170b into master May 9, 2018
@patrickvenetz patrickvenetz deleted the encode-emails-urlsafe branch May 9, 2018 18:52
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants