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

Change nextUri slug for every request #1660

Merged
merged 1 commit into from Oct 9, 2019

Conversation

@findepi
Copy link
Member

commented Oct 3, 2019

Fixes #1654

@cla-bot cla-bot bot added the cla-signed label Oct 3, 2019
@dain dain self-requested a review Oct 3, 2019
Copy link
Member

left a comment

This looks good to me, but I'd like to have @electrum take a look also.

{
public static Slug createNew()
{
return fromKey(randomUUID().toString().toLowerCase(ENGLISH).replace("-", ""));

This comment has been minimized.

Copy link
@dain

dain Oct 4, 2019

Member

Instead of going through UUID to get 128bits of random, just use:

byte[] randomBytes = new byte[16];
ThreadLocalRandom.current().nextBytes(randomBytes);
return  new Slug(randomBytes);

This comment has been minimized.

Copy link
@findepi

findepi Oct 4, 2019

Author Member

It's just moved from io.prestosql.dispatcher.QueuedStatementResource.Query#slug initialized, so I will change it in a separate commit.

This comment has been minimized.

Copy link
@findepi

findepi Oct 4, 2019

Author Member

I will go with SecureRandom though.

@dain dain requested a review from electrum Oct 4, 2019
@sopel39

This comment has been minimized.

Copy link
Member

commented Oct 4, 2019

What's the risk if we leave slug same for every request? Browsers for instance use the same cookie for the entire session duration and it's not posed as a great security risk?

@findepi

This comment has been minimized.

Copy link
Member Author

commented Oct 4, 2019

@sopel39 i added the rationale to the issue


public String makeSlug(long token)
{
return "XD" + hmac.hashLong(token).toString();

This comment has been minimized.

Copy link
@martint

martint Oct 4, 2019

Member

What's "XD"?

This comment has been minimized.

Copy link
@findepi

findepi Oct 4, 2019

Author Member

Fixed prefix to distinguish the slug, for troubleshooting purposes.
Previous slug version used "x" prefix.

This comment has been minimized.

Copy link
@dain

dain Oct 4, 2019

Member

I think I added so it would be easy to grep for. Maybe we make it Z this time?

This comment has been minimized.

Copy link
@findepi

findepi Oct 5, 2019

Author Member

it's y

@findepi

This comment has been minimized.

Copy link
Member Author

commented Oct 7, 2019

squashed

@findepi findepi force-pushed the findepi:sluggish branch from 8adf4d0 to 2ea2c91 Oct 7, 2019
public String makeSlug(Context context, long token)
{
// "y" is an arbitrary prefix distinguishing this slug version. Added for troubleshooting purposes.
return "y" + hmac.newHasher()

This comment has been minimized.

Copy link
@electrum

electrum Oct 7, 2019

Member

How about q or z?

This comment has been minimized.

Copy link
@findepi

findepi Oct 8, 2019

Author Member

How q or z is better than y?

{
// "y" is an arbitrary prefix distinguishing this slug version. Added for troubleshooting purposes.
return "y" + hmac.newHasher()
.putInt(requireNonNull(context, "context is null").ordinal())

This comment has been minimized.

Copy link
@electrum

electrum Oct 7, 2019

Member

No need for a null check here, since we aren't storing the value

This comment has been minimized.

Copy link
@findepi

findepi Oct 8, 2019

Author Member

i want informative exception message rather than just NPE without a message

@findepi findepi force-pushed the findepi:sluggish branch from fc14f8a to 5942416 Oct 8, 2019
@findepi findepi force-pushed the findepi:sluggish branch 2 times, most recently from a9c7d5b to 651109b Oct 8, 2019
@findepi findepi merged commit 52614f7 into prestosql:master Oct 9, 2019
2 checks passed
2 checks passed
Travis CI - Pull Request Build Passed
Details
verification/cla-signed
Details
@findepi findepi deleted the findepi:sluggish branch Oct 9, 2019
@findepi findepi added this to the 321 milestone Oct 9, 2019
@findepi findepi referenced this pull request Oct 9, 2019
4 of 6 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
5 participants
You can’t perform that action at this time.