Skip to content

Conversation

@thiskevinwang
Copy link
Contributor

@thiskevinwang thiskevinwang commented Jun 20, 2022

🔗 Relevant links

Related PR:

🗒️ What

🤷 Why

🛠️ How

📸 Design Screenshots

🧪 Testing

💭 Anything else?

@vercel
Copy link

vercel bot commented Jun 20, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
dev-portal ✅ Ready (Inspect) Visit Preview Jun 20, 2022 at 5:21PM (UTC)

@github-actions
Copy link

Some suggested prefixes and emojis that may help to write clear, actionable code review comments:

Praise 🙌 Question 🙋 Thought 💭 Blocker 🚧 Future 📌 Optional 🎨 Nitpick ⛏️
Expand for comment prefix descriptions
Prefix+Emoji Description
Praise 🙌 Use to highlight something positive. It's nice to try to leave one per review, but don't leave false praise just to leave one of these comments.
Question 🙋 Use to gain clarity from the code author. The conversation could evolve into any one of these other categories. Only the reviewer should resolve these comment threads.
Thought 💭 Use to share context, leave a breadcrumb, or share an idea that came up while reviewing.
Blocker 🚧 Use to request changes that block merging the current PR. Only the reviewer should resolve these comment threads.
Future 📌 Use to request changes that the code author can choose to address in the current PR or a follow-up one.
Optional 🎨 Use to suggest optional changes that you feel strongly about but ultimately defer to the code author to make a decision on. These can be comments that turn into valuable conversation starters for adopting new code styles, guidelines, or team practices.
Nitpick ⛏️ Use to suggest changes based on loose opinions or personal preferences. The difference between this and Optional 🎨  is how strong the code reviewer's opinion is.

@github-actions
Copy link

github-actions bot commented Jun 20, 2022

📦 Next.js Bundle Analysis

This analysis was generated by the next.js bundle analysis action 🤖

This PR introduced no changes to the javascript bundle 🙌

@brkalow
Copy link
Contributor

brkalow commented Jun 20, 2022

@thiskevinwang Can you elaborate a bit on the approach here and explain how we consider a URL to be "invalid"?

@thiskevinwang
Copy link
Contributor Author

@brkalow the approach here is on the naive side:

Given a path like...
/docs/upgrade%25'%20AND%202*3*8=6*8%20AND%20'zVVl'!='zVVl%25/upgrade-specific...n

...I'm decoding it once...
/docs/upgrade%' AND 2*3*8=6*8 AND 'zVVl'!='zVVl%/upgrade-specific

...and if it still contains % or whitespace, I'm considering that invalid, and returning an early 404

Comment on lines +123 to +128
// catch invalid URIs early
if (isInvalidURI(pathParts.join('/'))) {
return {
notFound: true,
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

💭 Can we expand on the approach / heuristic here for how we determine an invalid URL / why we are doing this? I think that will be a helpful breadcrumb for the future!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually thinking about this more, i'm curious the reasoning behind catching only 404's and returning notFound: true...

if (error.status === 404) {
return { notFound: true }
}

This class of bad requests cause our Content API to return a Vercel 400, so maybe it'd be simpler to include that in the detected status codes

@thiskevinwang thiskevinwang deleted the kevin/reject-requests branch August 5, 2022 17:47
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.

2 participants