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

feat: add getRequestFingerprint util #548

Merged
merged 0 commits into from
Nov 5, 2023
Merged

feat: add getRequestFingerprint util #548

merged 0 commits into from
Nov 5, 2023

Conversation

nandi95
Copy link
Contributor

@nandi95 nandi95 commented Sep 29, 2023

closes #536

@pi0 pi0 changed the title (feat): add getFingerprint util feat: add getRequestFingerprint util Oct 3, 2023
src/utils/request.ts Outdated Show resolved Hide resolved
@pi0 pi0 requested a review from danielroe October 3, 2023 17:36
@codecov
Copy link

codecov bot commented Oct 3, 2023

Codecov Report

Merging #548 (c0e2f6a) into main (b7aca96) will increase coverage by 0.40%.
The diff coverage is 100.00%.

❗ Current head c0e2f6a differs from pull request most recent head b7aca96. Consider uploading reports for the commit b7aca96 to get more accurate results

@@            Coverage Diff             @@
##             main     #548      +/-   ##
==========================================
+ Coverage   83.83%   84.23%   +0.40%     
==========================================
  Files          31       31              
  Lines        3563     3629      +66     
  Branches      543      550       +7     
==========================================
+ Hits         2987     3057      +70     
+ Misses        576      572       -4     
Files Coverage Δ
src/utils/request.ts 96.19% <100.00%> (+3.30%) ⬆️

@pi0
Copy link
Member

pi0 commented Oct 3, 2023

Thanks for working on this.

It seems a useful utility for security / rate-limiting purposes. (/cc @danielroe mentioned you if have any concerns to avoid introducing this utility as-is)

Considering this usage, i have market it as experimental for now and only enabled IP hashing by default (without trust on X-Forwarded-For).

Features (xfowarded/path/method/userAgent) can be enabled opt-in by default.

PR welcome for more improvements.

@nandi95
Copy link
Contributor Author

nandi95 commented Oct 3, 2023

This seems good to me. I don't get why have the path and method opt in. Unlike userAgent, they cannot be spoofed?

Side note, perhaps this or another utility can account for the whole request as is, including the query string, body and headers. Such a thing would be useful if someone had to de-duplicate requests (if that is even a thing) or perhaps some anonymised server side analytics.

src/utils/request.ts Outdated Show resolved Hide resolved
Comment on lines 245 to 247
const fingerprintString =
fingerprint.filter(Boolean).join("|") ||
Math.random().toString(36).slice(2);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
const fingerprintString =
fingerprint.filter(Boolean).join("|") ||
Math.random().toString(36).slice(2);
if (!fingerprint.length) {
throw new RangeError('At least one fingerprinting factor must be enabled.')
}
const fingerprintString = fingerprint.filter(Boolean).join("|");

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would rather have an error thrown here, warning the developer. Otherwise, they might end up with different fingerprint for the same request.
(Range error is somewhat applicable, but it can be an error too)

Copy link
Member

Choose a reason for hiding this comment

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

We might instead disable random behavior and return null allowing usage to handle it for custom error wdty? (usually h3 utils are errorless)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's better too than a random string. In this case, the return value should be dependent on the options' argument type. Just so the user doesn't have to assert that the fingerprint is in fact a string.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

shall I make this change?

@pi0
Copy link
Member

pi0 commented Oct 3, 2023

Side note, perhaps this or another utility can account for the whole request as is, including the query string, body and headers. Such a thing would be useful if someone had to de-duplicate requests (if that is even a thing) or perhaps some anonymised server side analytics.

For an analytics usecase, uniqueness of visitors is important yes, but i guess it might need to keep path unhashed otherwise it would be useless.

Mainly enabling these is dangerous for main purpose i could assume (security) because for example someone can change identity by iterating user agent or appending a query param to the end of url.

Once we had better vision (util is still in experimental state) we might improve defaults or have an option that enables all/group of defaults

@pull pull bot force-pushed the main branch 2 times, most recently from 613d281 to b7aca96 Compare October 24, 2023 14:11
@nandi95
Copy link
Contributor Author

nandi95 commented Oct 25, 2023

Opened a new PR from a branch that isn't named main as pull bot keeps nullifying the work.
#564
I think this can be closed.

@renovate renovate bot merged commit b7aca96 into unjs:main Nov 5, 2023
4 checks passed
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.

getRequestFingerprint
3 participants