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

Functions Framework prevents developers from handling invalid params #409

Closed
inlined opened this issue Dec 22, 2021 · 10 comments
Closed

Functions Framework prevents developers from handling invalid params #409

inlined opened this issue Dec 22, 2021 · 10 comments

Comments

@inlined
Copy link

inlined commented Dec 22, 2021

firebase/firebase-tools#3891 seems to have a root cause in the functions framework.

To reproduce without Firebase:

const express = require('express');
const app = express();

app.get('/404', (req, res) => res.send('failed to work'));
app.get('**', (req, res) => res.send('working'));
app.use((err, req, res, next) => res.redirect('/404'));

module.exports.app = app;

if (require.main == module) {
  app.listen(8080);
}

If you run the file locally (no functions framework) and curl http://localhost:8080/%CO the user is redirected to http://localhost:8080/404.

If you deploy the function and CURL the repo (temporarily) at https://us-central1-inlined-junkdrawer.cloudfunctions.net/param-encoding/%C0, the functions framework rejects the request before it ever hits customer code with Bad Request

@inlined
Copy link
Author

inlined commented Dec 22, 2021

CC @DevLucem , this will be the new home for firebase-tools#3891

@grant
Copy link
Contributor

grant commented Dec 22, 2021

Hi @inlined, thanks for the issue.

Can you please provide more details, including full repro steps in this issue (not linked issues)?

For example, the sample above uses Express and Express routing, not the Functions Framework.

@DevLucem
Copy link

Thank you @inlined

@grant Here is the most basic code from cloud functions hello world

exports.helloWorld = (req, res) => {
  let message = req.query.message || req.body.message || 'Hello World!';
  res.status(200).send(message);
};

For this case, the unauthenticated invocations option has to be enabled as it is a publicly accessible website
image

Using the trigger URL for any method should work and will work until you add %CO in the URL or any invalid param. These params should instead be handled in the hello world code and the function should work as normal as we are not decoding the params yet.

@grant grant assigned grant and unassigned inlined Mar 28, 2022
@grant
Copy link
Contributor

grant commented Mar 28, 2022

Hi @DevLucem,
Thanks for your patience.

Here's the full repro:


index.js

exports.helloWorld = (req, res) => {
  let message = req.query.message || req.body.message || 'Hello World!';
  res.status(200).send(message);
};

package.json

{
  "main": "index.js",
  "dependencies": {
    "@google-cloud/functions-framework": "^3.0.0"
  }
}
npx @google-cloud/functions-framework --target helloWorld
Serving function...
Function: helloWorld
Signature type: http
URL: http://localhost:8080/
URIError: Failed to decode param '%CO'
    at decodeURIComponent (<anonymous>)

The full curl reveals the stacktrace:

URIError: Failed to decode param &#39;%CO&#39;
at decodeURIComponent (<anonymous>)
at decode_param (.../</anonymous>node_modules/express/lib/router/layer.js:172:12)
at Layer.match (.../</anonymous>node_modules/express/lib/router/layer.js:148:15)
at matchLayer (.../</anonymous>node_modules/express/lib/router/index.js:580:18)
at next (.../</anonymous>node_modules/express/lib/router/index.js:220:15)
at expressInit (.../</anonymous>node_modules/express/lib/middleware/init.js:40:5)
at Layer.handle [as handle_request] (.../</anonymous>node_modules/express/lib/router/layer.js:95:5)
at trim_prefix (.../</anonymous>node_modules/express/lib/router/index.js:323:13)
at .../</anonymous>node_modules/express/lib/router/index.js:284:7
at Function.process_params (.../</anonymous>node_modules/express/lib/router/index.js:341:12)

It is expected that the function wrapper handled the HTTP request to produce the (req) object.

There currently is no possible way to intercept exceptions like invalid URLs. If you'd like there to be a way, can you please file a GitHub issue in the main FF repo? https://github.com/GoogleCloudPlatform/functions-framework


In your application, if you really wanted to, you can workaround this issue by providing a custom middleware, although that is not officially supported. Here's a general handler method, and post for middleware:

I'm going to close this issue out, but if there are pending questions, please let us know. Hope this info is helpful.

@grant grant closed this as completed Mar 28, 2022
@DevLucem
Copy link

Thanks for your response @grant.

The middleware solution you offered is not supported and cannot be a workaround as the URL is checked before it gets to any cloud function. @inlined code shows the middleware sample.

@devgeni
Copy link

devgeni commented Mar 30, 2022

I also have tried a middleware solution and I can confirm that it doesn't work.
The URIError is thrown before any middleware can catch it.

@grant
Copy link
Contributor

grant commented Mar 30, 2022

Thanks.

What would you like the dev experience to be ideally? Expected behavior? Can you write that out?

I'm hearing you'd like to still invoke the function, but the server cannot accept a bad request. An error handler? Express sets up the router and processes params a bit out of our control.

@grant grant reopened this Mar 30, 2022
@devgeni
Copy link

devgeni commented Mar 31, 2022

I expected for middleware workaround to solve this problem and honestly I can not come up with any idea of how this should behave.
It looks like this is in the production code of thousands of devs.
So we just can't intercept this exception on any level?

@grant grant removed their assignment Apr 6, 2022
@grant
Copy link
Contributor

grant commented Apr 15, 2022

To solve this, I think we'd need to support a lifecycle hook to handle BAD_REQUESTs to the function.

I wrote my thoughts here: GoogleCloudPlatform/functions-framework#56

Similar: https://stackoverflow.com/a/31576777

@josephlewis42
Copy link
Contributor

Closing this as it's tracked upstream against the FF spec.

@josephlewis42 josephlewis42 closed this as not planned Won't fix, can't repro, duplicate, stale Aug 29, 2023
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

No branches or pull requests

5 participants