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

Update auth config to allow authorized images (#400) #422

Closed

Conversation

tylerhoward15
Copy link
Contributor

#400

Customer images were not loading after adding auth. This was because authorized requests were only allowed if they came from the /dashboard route. However, when fetching customer images, they were requested from the route /customers.

So by adding a list of protected urls, we ensure they're all available when logged in, and they're inaccessible when not logged in.

Copy link

vercel bot commented Nov 10, 2023

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

Name Status Preview Comments Updated (UTC)
next-learn-starter ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 16, 2023 3:34pm
next-seo-starter ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 16, 2023 3:34pm

Copy link

vercel bot commented Nov 10, 2023

@Tyler98ky is attempting to deploy a commit to the Vercel Team on Vercel.

A member of the Team first needs to authorize it.

@lnicepei
Copy link

Had same issue. Adding @tylerhoward15's code fixed broken images

@emapeire
Copy link

emapeire commented Nov 12, 2023

It works for me!

Comment on lines +14 to +17
const protectedPaths = ['/dashboard', '/customers', '/invoices'];
const isProtectedPath = protectedPaths.some((path) =>
nextUrl.pathname.startsWith(path),
);
Copy link
Collaborator

@delbaoliveira delbaoliveira Nov 16, 2023

Choose a reason for hiding this comment

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

Hey @tylerhoward15, thank you for the contribution. This was resolved in this PR: #355

Are you still seeing broken images? I'm not able to reproduce.

In either case:

  • The protected pathname starts with /dashboard. E.g. /dashboard/customers. So startsWith would evaluate to false for /customers.
  • I think it's slightly better practice to protect a route higher up (/dashboard), so that if we add or change the name of a child segment, they'd automatically be protected.

@tylerhoward15
Copy link
Contributor Author

Hey @delbaoliveira, thanks for checking in on this!

When I first encountered this issue and opened this PR (November 9th), #355 appears to have already been merged (November 3rd). However, myself and a few others were definitely still having assets not loading.

During troubleshooting, I logged out the nextUrl sent to the authorized callback. What I discovered was that not all requests were being sent prefixed with /dashboard. The requests that were being rerouted included /invoices and /customers, and in particular, the customers route where the images were served was causing our issues.

However, I just pulled a copy of the latest changes to the repo, and tested locally and deployed to Vercel adding logs again, and those requests all seem to be prefixed properly now! I am however still seeing 302 redirects for the /favicon.ico requests, which, I'll provide screenshots below for.

So it seems as though maybe there was a bug where the application wasn't prefixing requests properly with dashboard, but that has since been resolved since this issue was opened. That seems the most likely scenario to me, but just to clarify, there were still issues after #355 for some of us at least.

As for the /favicon redirects, it seems to be a slightly different issue that I haven't looked into, but just wanted to let you know, and I can open a separate issue for that if it feels relevant.

/favicon.ico redirect logs
Screenshot 2023-11-16 at 12 59 21 PM

@delbaoliveira
Copy link
Collaborator

However, I just pulled a copy of the latest changes to the repo, and tested locally and deployed to Vercel adding logs again, and those requests all seem to be prefixed properly now!

Glad this is resolved 😌

As for the /favicon redirects, it seems to be a slightly different issue that I haven't looked into, but just wanted to let you know, and I can open a separate issue for that if it feels relevant.

Thank you, I'll look into the /favicon issue 🙏🏼

I'm sure there are projects where people haven't updated and might still be seeing this issue. Since I'm not able to reproduce in the latest, I'll close the PR for now, thank you for bringing it up. Appreciate it!

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.

4 participants