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

Add client option to disable redirects #21024

Merged
merged 1 commit into from Jan 10, 2024
Merged

Conversation

dianatatar
Copy link
Contributor

@dianatatar dianatatar commented Oct 3, 2023

Presto clients can attempt to follow a redirect from an untrusted server, adding option to disable redirect as a security improvement.

Description

Add client option to disable redirects

Motivation and Context

Presto clients can attempt to follow a redirect from an untrusted server, adding option to disable redirect as a security improvement
Fixes advisory GHSA-xm7x-f3w2-4hjm

Impact

Adding option to disable Presto clients following a redirect. This is an opt in, by default the current client behavior does not change

Test Plan

Tested locally with CLI and Presto Jdbc client by sending a server redirect (307/308 http code) and confirming clients will not follow the redirect if option to follow redirects is disabled

Contributor checklist

  • Please make sure your submission complies with our development, formatting, commit message, and attribution guidelines.
  • PR description addresses the issue accurately and concisely. If the change is non-trivial, a GitHub Issue is referenced.
  • Documented new properties (with its default value), SQL syntax, functions, or other functionality.
  • If release notes are required, they follow the release notes guidelines.
  • Adequate tests were added if applicable.
  • CI passed.

Release Notes

Please follow release notes guidelines and fill in the release notes below.

== RELEASE NOTES ==

General Changes
* Add client option to disable redirects

@dianatatar dianatatar requested a review from a team as a code owner October 3, 2023 18:12
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Oct 3, 2023

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: dianatatar / name: Diana Tatar (45727d6)

@skairali
Copy link
Member

skairali commented Oct 6, 2023

@dianatatar what happens when presto://localhost:8080/blackhole?followRedirects=ANINVALIDOPTION is given here?

it could be a typo as well like presto://localhost:8080/blackhole?followRedirects=false1.

What's the behaviour?

@dianatatar
Copy link
Contributor Author

@dianatatar what happens when presto://localhost:8080/blackhole?followRedirects=ANINVALIDOPTION is given here?

it could be a typo as well like presto://localhost:8080/blackhole?followRedirects=false1.

What's the behaviour?

Connection properties take in several checks when initiated, one of which is a converter which checks if the URI parameter value is as expected. followRedirects uses a boolean converter which checks that the value is either true or false and throws otherwise. I added an extra assert in TestPrestoDriverUri to confirm this behavior.

@github-actions
Copy link

github-actions bot commented Oct 11, 2023

Codenotify: Notifying subscribers in CODENOTIFY files for diff b5a403b...45727d6.

Notify File(s)
@steveburnett presto-docs/src/main/sphinx/installation/jdbc.rst

Copy link
Contributor

@steveburnett steveburnett left a comment

Choose a reason for hiding this comment

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

LGTM! (docs)

@tcherel
Copy link

tcherel commented Nov 21, 2023

Hi.
Any ideas if and when (rough idea) this might be delivered and available in a new presto JDBC GA driver?
Thanks.

@tdcmeehan
Copy link
Contributor

@skairali any further comments?

@tcherel
Copy link

tcherel commented Dec 4, 2023

@tdcmeehan @skairali any updates or some sort of rough ETA?
Thanks.

1 similar comment
@tcherel
Copy link

tcherel commented Jan 6, 2024

@tdcmeehan @skairali any updates or some sort of rough ETA?
Thanks.

@skairali
Copy link
Member

skairali commented Jan 7, 2024

@dianatatar (1) Could you please let me know what's the default value of the parameter? I would recommend the configuration to work AS IS and only take affect if its explicitly provided

(2)Please also resolve conflicts and rebase

@tdcmeehan @tcherel I am approving this PR conditionally that @dianatatar takes care of (1) and (2)

Copy link
Member

@skairali skairali left a comment

Choose a reason for hiding this comment

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

Marking my approval as commented above

@tcherel
Copy link

tcherel commented Jan 8, 2024

Thanks @skairali
@dianatatar any chances you can provide what @skairali requested above?
Thanks.

Presto clients can attempt to follow a redirect from an untrusted server, adding option to disable redirect as a security improvement.
@dianatatar
Copy link
Contributor Author

@skairali the configuration parameter has a default value of true which means it will follow redirects and is also the default behavior. I've rebased the PR and resolved the conflicts. @tcherel

@skairali skairali merged commit 5a5044b into prestodb:master Jan 10, 2024
57 checks passed
@wanglinsong wanglinsong mentioned this pull request Feb 12, 2024
64 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

5 participants