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

Default validator doesn't accept Nginx generated request_id #99

Open
shtoltz opened this issue Nov 26, 2024 · 3 comments
Open

Default validator doesn't accept Nginx generated request_id #99

shtoltz opened this issue Nov 26, 2024 · 3 comments

Comments

@shtoltz
Copy link

shtoltz commented Nov 26, 2024

After change in #92 request_id generated by Nginx doesn't pass default validation.

Example log from nginx:

[c10f7ebebd95e5bb8749430d3485370c] 192.168.131.1 - - [26/Nov/2024:17:06:43 +0000] "GET / HTTP/1.1" 404 22 "-" "Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/131.0.0.0 Safari/537.36" "-"

With validator's logic before change:

>>> bool(uuid.UUID('c10f7ebebd95e5bb8749430d3485370c', version=4))
True
>>> uuid.UUID('c10f7ebebd95e5bb8749430d3485370c', version=4)
UUID('c10f7ebe-bd95-45bb-8749-430d3485370c')

fragment e5bb has changed to 45bb but request_id is still valid

With current validator's logic:

>>> uuid.UUID('c10f7ebebd95e5bb8749430d3485370c').version == 4
False
>>> uuid.UUID('c10f7ebebd95e5bb8749430d3485370c')
UUID('c10f7ebe-bd95-e5bb-8749-430d3485370c')

According to the docs:

$request_id
unique request identifier generated from 16 random bytes, in hexadecimal (1.11.0)
@sondrelg
Copy link
Member

sondrelg commented Dec 6, 2024

I see. That's unfortunate; nginx generated request IDs seem commonly enough used to warrant trying to support them. Would you be interested in trying to create a fix @shtoltz?

IIRC, there was a problem with false positives before the fix - but there's probably a way to fix both.

@JonasKs
Copy link
Member

JonasKs commented Dec 6, 2024

For what it's worth, the current validator also don't work with OTEL generated trace IDs, which has the same format as nginx.

Always fixable with something like this, though:

def is_valid_uuid(original_guid: str) -> bool:
    try:
        return bool(uuid.UUID(original_guid, version=4))
    except Exception:
        return False

m = Middleware(CorrelationIdMiddleware, validator=is_valid_uuid)

I'm in favor of having #92 reverted, though. 👍

@sondrelg
Copy link
Member

sondrelg commented Dec 6, 2024

Perhaps we could revert, but change the name of the function and document the fact that it's more loosely validated than for "proper" UUIDv4s

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

No branches or pull requests

3 participants