Skip to content

fix: don't ever execute middleware in lambda #2249

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

Merged
merged 3 commits into from
Aug 10, 2023
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 34 additions & 0 deletions cypress/e2e/middleware/preview.cy.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
describe('Preview Mode', () => {
it('enters and exits preview mode', () => {
Cypress.Cookies.debug(true)
cy.getCookies().then((cookie) => cy.log('cookies', cookie))

cy.intercept('/previewTest', (req) => {
req.continue((res) => {
expect(res.headers?.['x-middleware-executed']).to.equal('true')
})
}).as('previewTestVisit')

// preview mode is off by default
cy.visit('/previewTest')
cy.findByText('Is preview? No', { selector: 'h1' })

// enter preview mode
cy.request('/api/enterPreview').then((response) => {
expect(response.body).to.have.property('name', 'preview mode')
})

// exptected content is rendered
cy.visit('/previewTest')
cy.findByText('Is preview? Yes!', { selector: 'h1' })

// exit preview mode
cy.request('/api/exitPreview')
cy.visit('/previewTest')
cy.findByText('Is preview? No', { selector: 'h1' })

// we should hit /previewTest 3 times (before entering preview, after entering preview, after exiting preview)
// this assertion is mainly to ensure interception works and assertion on response header is made
cy.get('@previewTestVisit.all').should('have.length', 3)
})
})
Comment on lines +1 to +34
Copy link
Contributor Author

@pieh pieh Aug 10, 2023

Choose a reason for hiding this comment

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

This test does assert things correctly, but the version of Next used in middleware demo actually doesn't have the bug included, so the fix is only relevant to older next versions which we don't run test against currently :/

I think it's still worthwhile to include the test, but we should figure out test setup to run against more versions of next.js.

[email protected] does fail this test without the fix ( https://cloud.cypress.io/projects/yn8qwi/runs/2299/overview?roarHideRunsWithDiffGroupsAndTags=1 ) but with setup as-is we won't be using that version of next :/

Choose a reason for hiding this comment

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

It's good to have this like you mentioned and if we get around to running these tests on older versions, this will definitely be helpful.

29 changes: 18 additions & 11 deletions demos/middleware/middleware.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,11 +15,11 @@ export async function middleware(req: NextRequest) {
headers.set('x-hello', 'world')
return NextResponse.next({
request: {
headers
}
headers,
},
})
}

const request = new MiddlewareRequest(req)

// skipMiddlewareUrlNormalize next config option is used so we have to try to match both html path and data blob path
Expand All @@ -41,12 +41,11 @@ export async function middleware(req: NextRequest) {
// skipMiddlewareUrlNormalize next config option is used so we have to try to match both html path and data blob path
if (pathname.startsWith('/request-rewrite') || pathname.endsWith('/request-rewrite.json')) {
// request.rewrite() should return the MiddlewareResponse object instead of the Response object.
const res = await request.rewrite('/static-rewrite',
{
const res = await request.rewrite('/static-rewrite', {
headers: {
'x-rewrite-test': 'hello',
'x-rewrite-test-2': 'hello-2'
}
'x-rewrite-test-2': 'hello-2',
},
})
const message = `This was static (& escaping test &) but has been transformed in ${req.geo?.city}`

Expand Down Expand Up @@ -90,7 +89,7 @@ export async function middleware(req: NextRequest) {
return response
}

if(pathname.startsWith('/matcher-cookie')) {
if (pathname.startsWith('/matcher-cookie')) {
response = NextResponse.next()
response.cookies.set('missingCookie', 'true')
return response
Expand All @@ -109,6 +108,13 @@ export async function middleware(req: NextRequest) {
return response
}

if (pathname.startsWith('/previewTest')) {
response = NextResponse.next()

response.headers.set('x-middleware-executed', 'true')
return response
}

if (pathname.includes('locale-preserving-rewrite')) {
return NextResponse.rewrite(new URL('/locale-test', req.url))
}
Expand Down Expand Up @@ -167,8 +173,8 @@ export const config = {
'/:all*/locale-preserving-rewrite',
'/cookies/:path*',
{ source: '/static' },
{source: '/request-rewrite' },
{ source: '/matcher-cookie'},
{ source: '/request-rewrite' },
{ source: '/matcher-cookie' },
{ source: '/shows/((?!99|88).*)' },
{
source: '/conditional',
Expand All @@ -186,8 +192,9 @@ export const config = {
{
type: 'cookie',
key: 'missingCookie',
}
},
],
},
'/previewTest',
],
}
6 changes: 6 additions & 0 deletions demos/middleware/pages/api/enterPreview.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
export default async function preview(req, res) {
// Enable Preview Mode by setting the cookies
res.setPreviewData({})

res.status(200).json({ name: 'preview mode' })
}
8 changes: 8 additions & 0 deletions demos/middleware/pages/api/exitPreview.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
export default async function exit(_, res) {
// Exit the current user from "Preview Mode". This function accepts no args.
res.clearPreviewData()

// Redirect the user back to the index page.
res.writeHead(307, { Location: '/' })
res.end()
}
3 changes: 3 additions & 0 deletions demos/middleware/pages/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,9 @@ export default function Home() {
<p>
<Link href="/headers">Adds `x-hello` request header to a rewrite</Link>
</p>
<p>
<Link href="/previewTest">Preview mode</Link>
</p>
</main>
</div>
)
Expand Down
25 changes: 25 additions & 0 deletions demos/middleware/pages/previewTest.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
import Link from 'next/link'

const StaticTest = ({ isPreview }) => {
return (
<div>
<h1>Is preview? {isPreview ? 'Yes!' : 'No'}</h1>
<p>
<a href={isPreview ? '/api/exitPreview' : '/api/enterPreview'}>
{isPreview ? 'Exit Preview' : 'Enter Preview'}
</a>
</p>
<Link href="/">Go back home</Link>
</div>
)
}

export const getStaticProps = async ({ preview }) => {
return {
props: {
isPreview: Boolean(preview),
},
}
}

export default StaticTest
8 changes: 8 additions & 0 deletions packages/runtime/src/templates/server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ interface NetlifyConfig {
revalidateToken?: string
}

// eslint-disable-next-line max-lines-per-function
const getNetlifyNextServer = (NextServer: NextServerType) => {
class NetlifyNextServer extends NextServer {
private netlifyConfig: NetlifyConfig
Expand Down Expand Up @@ -118,6 +119,13 @@ const getNetlifyNextServer = (NextServer: NextServerType) => {
}
}

// eslint-disable-next-line class-methods-use-this, require-await
async runMiddleware(): Promise<{ finished: boolean }> {

Choose a reason for hiding this comment

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

return {
finished: false,
}
}

private getNetlifyPathsForRoute(route: string): string[] {
const { i18n } = this.nextConfig
const { routes, dynamicRoutes } = this.netlifyPrerenderManifest
Expand Down