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

Scala Stream Collector: allow users to disable the default redirect endpoint #4211

Closed
dilyand opened this issue Oct 4, 2019 · 4 comments
Closed

Comments

@dilyand
Copy link
Contributor

dilyand commented Oct 4, 2019

The Javascript tracker allows for click tracking through open redirects. There are concerns that this functionality might be abused by malicious third parties. Even if SSC users are not making use of the feature in the JS tracker, they are still potentially vulnerable as the collector would accept requests to the default redirect endpoint.

@dilyand dilyand modified the milestones: r17, R117 Referer parser Oct 4, 2019
peel added a commit that referenced this issue Oct 4, 2019
Add a new flag `collector.allowRedirects` that when set to false will
prohibit (`410 Gone`) any redirects using `r/` prefix url.
peel added a commit that referenced this issue Oct 4, 2019
Add a new flag `collector.allowRedirects` that when set to false will
prohibit (`410 Gone`) any redirects using `r/` prefix url.
@dilyand
Copy link
Contributor Author

dilyand commented Oct 7, 2019

It would be great if we can still allow redirects, just not via the default route.

The default route is public, which makes it easy to abuse. However, a custom redirect path will be much harder to guess.

We can't use custom path mappings for this, because the custom path will have to be mapped to some default. But if we make the redirect path configurable, then users will be able to have the functionality in a secure way.

Seeing how we're already adding a new item to the config, what if instead of a Boolean it's just the redirect path to be used -- and if it's not specified then redirects are not possible? I wonder if this would be much harder to implement.

@peel
Copy link
Contributor

peel commented Oct 7, 2019

@dilyand to me the only problem here is backwards compatibility:

  • I deliberately set a 4xx response on the old route to make submitting events to collector invalid, otherwise ${OLD_PREFIX}/${PROTOCOL_VERSION} is a perfectly valid event that gets tracked. Can't be done with fully configurable or we need to hardcode the old default somewhere (as is it is now) until all the users have migrated.
  • With fully configurable we disable the old r/ route by default (should we?) so upon upgrade config changes are necessary.

What are your thoughts on the two?

@dilyand
Copy link
Contributor Author

dilyand commented Oct 7, 2019

I think what we have now in the proposed 0.17.0 is good enough. It might be worthwhile though to open an issue for future versions, where we give users the ability to only use a custom redirect path (ie, disabling the default path but not redirects as such). And have the discussion on backward compatibility there, if we ever pick up the issue again.

peel added a commit that referenced this issue Oct 7, 2019
…ndpoint (fixes #4211)

Add a new flag `collector.allowRedirects` that when set to false will
prohibit (`410 Gone`) any redirects using `r/` prefix url.
peel added a commit that referenced this issue Oct 7, 2019
…ndpoint (fixes #4211)

Add a new flag `collector.allowRedirects` that when set to false will
prohibit (`410 Gone`) any redirects using `r/` prefix url.
peel added a commit that referenced this issue Oct 8, 2019
Scala Stream Collector: allow disabling redirects (fixes #4211)
peel added a commit that referenced this issue Oct 8, 2019
…ndpoint (fixes #4211)

Add a new flag `collector.allowRedirects` that when set to false will
prohibit (`410 Gone`) any redirects using `r/` prefix url.
peel added a commit that referenced this issue Oct 17, 2019
…ndpoint (fixes #4211)

Add a new flag `collector.allowRedirects` that when set to false will
prohibit (`410 Gone`) any redirects using `r/` prefix url.
@peel peel changed the title Scala Stream Collector: Allow users to disable the default redirect endpoint Scala Stream Collector: allow users to disable the default redirect endpoint Oct 18, 2019
peel added a commit that referenced this issue Oct 18, 2019
…ndpoint (fixes #4211)

Add a new flag `collector.allowRedirects` that when set to false will
prohibit (`410 Gone`) any redirects using `r/` prefix url.
peel added a commit that referenced this issue Oct 18, 2019
…ndpoint (fixes #4211)

Add a new flag `collector.enableDefaultRedirect` that when set to false will
prohibit (`404 Not Found`) any redirects using `r/` prefix url.
peel added a commit that referenced this issue Oct 18, 2019
…ndpoint (fixes #4211)

Add a new flag `collector.enableDefaultRedirect` that when set to false will
prohibit (`404 Not Found`) any redirects using `r/` prefix url.
lukeindykiewicz pushed a commit that referenced this issue Oct 22, 2019
…ndpoint (closes #4211)

Add a new flag `collector.enableDefaultRedirect` that when set to false will
prohibit (`404 Not Found`) any redirects using `r/` prefix url.
lukeindykiewicz pushed a commit that referenced this issue Oct 22, 2019
…ndpoint (closes #4211)

Add a new flag `collector.enableDefaultRedirect` that when set to false will
prohibit (`404 Not Found`) any redirects using `r/` prefix url.
lukeindykiewicz pushed a commit that referenced this issue Oct 23, 2019
…ndpoint (closes #4211)

Add a new flag `collector.enableDefaultRedirect` that when set to false will
prohibit (`404 Not Found`) any redirects using `r/` prefix url.
lukeindykiewicz pushed a commit that referenced this issue Oct 24, 2019
…ndpoint (closes #4211)

Add a new flag `collector.enableDefaultRedirect` that when set to false will
prohibit (`404 Not Found`) any redirects using `r/` prefix url.
lukeindykiewicz pushed a commit that referenced this issue Oct 24, 2019
…ndpoint (closes #4211)

Add a new flag `collector.enableDefaultRedirect` that when set to false will
prohibit (`404 Not Found`) any redirects using `r/` prefix url.
lukeindykiewicz pushed a commit that referenced this issue Oct 25, 2019
…ndpoint (closes #4211)

Add a new flag `collector.enableDefaultRedirect` that when set to false will
prohibit (`404 Not Found`) any redirects using `r/` prefix url.
lukeindykiewicz pushed a commit that referenced this issue Oct 28, 2019
…ndpoint (closes #4211)

Add a new flag `collector.enableDefaultRedirect` that when set to false will
prohibit (`404 Not Found`) any redirects using `r/` prefix url.
lukeindykiewicz pushed a commit that referenced this issue Nov 7, 2019
…ndpoint (closes #4211)

Add a new flag `collector.enableDefaultRedirect` that when set to false will
prohibit (`404 Not Found`) any redirects using `r/` prefix url.
lukeindykiewicz pushed a commit that referenced this issue Nov 8, 2019
…ndpoint (closes #4211)

Add a new flag `collector.enableDefaultRedirect` that when set to false will
prohibit (`404 Not Found`) any redirects using `r/` prefix url.
lukeindykiewicz pushed a commit that referenced this issue Nov 15, 2019
…ndpoint (closes #4211)

Add a new flag `collector.enableDefaultRedirect` that when set to false will
prohibit (`404 Not Found`) any redirects using `r/` prefix url.
@chuwy chuwy closed this as completed in c444367 Dec 3, 2019
lukeindykiewicz pushed a commit to snowplow/stream-collector that referenced this issue Jun 4, 2020
…/snowplow#4211)

Add a new flag `collector.enableDefaultRedirect` that when set to false will
prohibit (`404 Not Found`) any redirects using `r/` prefix url.
lukeindykiewicz pushed a commit to snowplow/stream-collector that referenced this issue Jun 5, 2020
…/snowplow#4211)

Add a new flag `collector.enableDefaultRedirect` that when set to false will
prohibit (`404 Not Found`) any redirects using `r/` prefix url.
lukeindykiewicz pushed a commit to snowplow/stream-collector that referenced this issue Jun 5, 2020
…/snowplow#4211)

Add a new flag `collector.enableDefaultRedirect` that when set to false will
prohibit (`404 Not Found`) any redirects using `r/` prefix url.
lukeindykiewicz pushed a commit to snowplow/stream-collector that referenced this issue Jun 5, 2020
…/snowplow#4211)

Add a new flag `collector.enableDefaultRedirect` that when set to false will
prohibit (`404 Not Found`) any redirects using `r/` prefix url.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants