Conversation
Backend keys with .. in them have been added to the regex for patterns in keys that are denied.
|
The PR changelog entry failed validation: Changelog entry not found in the PR body. Please add a "no-changelog" label to the PR, or changelog lines starting with |
| var denyPatterns = []string{ | ||
| "//", | ||
| "..", | ||
| } |
There was a problem hiding this comment.
Are we sure this is the exhaustive list? Is . valid? What about ../. or ..//?
There was a problem hiding this comment.
I can confirm that . is an additional case we need to cover here. I was unable to reproduce this issue with a combination of . and /, but if we can be more restrictive that would be best.
edit** could we require at least one alpha or numeric character to be present?
There was a problem hiding this comment.
This definitely isn't exhaustive. . is valid, however, because we create .locks pretty regularly. The other two will be caught by this because this is just doing a simple bytes.Contains.
There was a problem hiding this comment.
We need to protect against the case where . is the entire name. I edited my prior comment, but in general I think we should just require at least one alpha or numeric character to be present.
There was a problem hiding this comment.
I'm thinking that we should adjust this so that it's the following:
.is the full name..is the full name../is found anywhere in the string//is found anywhere in the string.
Thoughts?
There was a problem hiding this comment.
Have we considered just running path.Clean() on this thing and seeing if the output matches the input?
There was a problem hiding this comment.
It sounds like what we are trying to say is:
- A path component can't be empty (
//and variants) - A path component can't be solely composed of dots or other special characters (must have a letter?)
I don't mind landing the change, but I'm unclear why we care. It's not like these are actual filepaths or would be resolved as such. Could anyone give a bit more background on this?
There was a problem hiding this comment.
I tried path.Clean(). I imagine I could make it work, but it doesn't quite work as well when there's no leading slash. I'm a little hesitant to munge the string too much to force a match.
There was a problem hiding this comment.
I've adjusted this to use some fancier regexes that will allow ..string as a name, which the UI is able to parse without issue.
fspmarshall
left a comment
fspmarshall
left a comment
There was a problem hiding this comment.
LGTM once /path/to/./val and /path/to/. are covered.
|
The PR changelog entry failed validation: Changelog entry not found in the PR body. Please add a "no-changelog" label to the PR, or changelog lines starting with |
mdwn
added
the
no-changelog
mdwn
removed
the
no-changelog
|
The PR changelog entry failed validation: Changelog entry not found in the PR body. Please add a "no-changelog" label to the PR, or changelog lines starting with |
public-teleport-github-review-bot
Bot
removed request for
EdwardDowling and
smallinsky
6 participants
Backend keys with .. in them have been added to the regex for patterns in keys that are denied.
changelog: Resources named
.and..are no longer allowed. Please review the resources in your Teleport instance and rename any resources with these names before upgrading.