-
Notifications
You must be signed in to change notification settings - Fork 31
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: Improved hostname validator #52
Conversation
@@ -124,7 +120,7 @@ public function isValid(mixed $value): bool | |||
} | |||
|
|||
// If every section matched; allow | |||
if($matchesAmount === \count($allowedSections)) { | |||
if ($matchesAmount === \count($allowedSections)) { | |||
return true; | |||
} | |||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the last line with return true
might be risky, maybe we can use this PR to change it. If it return false, we can avoid the last return false;
statement and minimize the code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The last return true is not regarding foreach, but regarding if. If no allowList is provided, return true. As soon as you go inside the if, it ends with a return false.
Anyway, I rewrote the method a little. The logic is still exactly the same, but instead of having scarry return true
at the end, it is now under if statement to make the code cleaner. Please let me know if it looks better now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now we also allow
*
orvercel.*
as hostnames.Issue: appwrite/appwrite#2939
Inside Appwrite,
*
is not actually returned as a header, instead, a specific domain is found and passed in the header. No need to worry about the security part, Apwrite handles it. Regarding "Secure by default", we can show noticeable alert saying*
shouldn't be used for reasons of being lazy, instead, only if your project requires this configuration (for instance Wordpress plugin).Regarding
vercel.*
, I don't think anyone would use it.. But that doesn't really matter we shouldn't support it if we can. I would say same as previous, could be insecure, but people are aware because they type it in.