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

fix some safe slug patterns #856

Merged
merged 2 commits into from
Oct 2, 2024
Merged

Conversation

minrk
Copy link
Member

@minrk minrk commented Oct 2, 2024

  • escape - in label pattern. Lacking this meant \.-_ was the range which included @, causing emails to not get escaped
  • is_valid_object_name now produces a valid object name for all cases, adding restrictions:
    • start with letter (numbers not allowed)
    • don't allow '.'
    • length 63
  • _extract_safe_name now satisfies the "starts with a letter" rule for RFC1035 label names

improved test coverage for several affected cases

- escape `-` in label pattern
- don't accept `.` in object names, to allow satisfying strict RFC 1035/1123
@minrk minrk added the bug label Oct 2, 2024
Copy link
Member

@consideRatio consideRatio left a comment

Choose a reason for hiding this comment

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

Thank you, seems ok to merge - is it ready to merge?

@minrk
Copy link
Member Author

minrk commented Oct 2, 2024

Yeah, I think so. Can do another quick beta after merging

@consideRatio consideRatio merged commit 31b9332 into jupyterhub:main Oct 2, 2024
11 checks passed
@consideRatio
Copy link
Member

Thank you for working this so thoroughly Min!!

@consideRatio
Copy link
Member

I realized just after merge, is docs updates relevant also?

@minrk minrk mentioned this pull request Oct 2, 2024
@minrk
Copy link
Member Author

minrk commented Oct 2, 2024

I realized just after merge, is docs updates relevant also?

No, the docs don't go into the details of exactly what the patterns are. This makes the docs accurate, where some escapes were not applied when they should be.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants