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

Generate URL-safe SGIDs #98

Merged
merged 1 commit into from
Apr 16, 2017
Merged

Generate URL-safe SGIDs #98

merged 1 commit into from
Apr 16, 2017

Conversation

georgeclaghorn
Copy link
Contributor

@georgeclaghorn georgeclaghorn commented Mar 22, 2017

We use SGIDs in some URLs sent via email from Basecamp. Since Basecamp 3’s launch, we’ve heard reports of broken links in emails from customers whose mail clients decode slashes in URL-encoded SGIDs.

For example, errant mail clients turn an SGID such as (a) below, which Rails parses as one path segment, into (b), which Rails parses as two path segments.

(a) BAh7B0kiCGdpZAY6BkVUSSInZ2lkOi8vYmN4L1BlcnNvbi8xMTUxODY%2FZXhwaXJlc19pbgY7AFRJIg9leHBpcmVzX2F0BjsAVDA=--ae5e44055262447fdbf5d5d39d5120cfa7d5fbe6
(b) BAh7B0kiCGdpZAY6BkVUSSInZ2lkOi8vYmN4L1BlcnNvbi8xMTUxODY/ZXhwaXJlc19pbgY7AFRJIg9leHBpcmVzX2F0BjsAVDA=--ae5e44055262447fdbf5d5d39d5120cfa7d5fbe6

Unfortunately, we’ve been unable to identify exactly which mail clients mangle SGIDs in this way.

Global ID already ensures unsigned GIDs are safe for use in URLs. To circumvent problematic mail clients, it can do the same for signed GIDs. This PR implements a custom message verifier that prefers URL-safe base64 encoding, GlobalID::Verifier, and makes it the default for SGID generation. GlobalID::Verifier accepts SGIDs generated with ActiveSupport::MessageVerifier for backwards compatibility.

/cc @jeremy

@georgeclaghorn
Copy link
Contributor Author

I didn’t realize that Global ID supports Rails 4.1, where this isn’t so tractable. ActiveSupport::MessageVerifier doesn’t support overriding methods to customize the encoding/decoding strategy in 4.1.

@rafaelfranca
Copy link
Member

Given that Rails 4.1 is not maintained anymore, we can drop support to it now. We just need to bump globalid to 0.4.0

@georgeclaghorn
Copy link
Contributor Author

Great! I’ve still got a failure with 4.2 on Ruby 1.9.3 to address. I’ll look into it tomorrow.

@georgeclaghorn
Copy link
Contributor Author

Tests are passing against Rails 4.2 and 5.0 now. All set?

@kaspth kaspth merged commit 7c3b42e into rails:master Apr 16, 2017
@kaspth
Copy link
Contributor

kaspth commented Apr 16, 2017

Yup! Going to remove the 4.1 build env now and then release this.

@kaspth
Copy link
Contributor

kaspth commented Apr 16, 2017

Think the GlobalID builds are waiting for the Rails builds to finish before starting, so postponing the release to later today. Got most of it wired up though and smoke tested using the Active Job tests with the 0.4.0 source.

@kaspth
Copy link
Contributor

kaspth commented Apr 16, 2017

It's out! https://rubygems.org/gems/globalid/versions/0.4.0https://github.com/rails/globalid/releases/tag/v0.4.0

@georgeclaghorn georgeclaghorn deleted the urlsafe-sgids branch April 16, 2017 19:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants