Skip to content

regression: parse urlencoded body as string when there is no specific key(payload) #35823

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.

Sign up for GitHub

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
ggazzo merged 3 commits into from
Apr 17, 2025
Merged

regression: parse urlencoded body as string when there is no specific key(payload) #35823

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f2afe16
regression: parse urlencoded body as string when there is no specific…
Apr 16, 2025
ebef241
Merge branch 'develop' into regression/hono-parse-urlencoded
Apr 16, 2025
b4740fd
Merge branch 'develop' into regression/hono-parse-urlencoded
Apr 17, 2025
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
2 changes: 1 addition & 1 deletion apps/meteor/app/api/server/router.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -166,7 +166,7 @@ export class Router<
// eslint-disable-next-line no-empty
} catch {}

return { ...overrideBodyParams };
return {};
}

private parseQueryParams(request: HonoRequest) {
Expand Down
19 changes: 14 additions & 5 deletions apps/meteor/app/integrations/server/api/api.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,7 @@ async function executeIntegrationRest() {
}
const content_raw = Buffer.concat(buffers).toString('utf8');
const protocol = `${this.request.headers.get('x-forwarded-proto')}:` || 'http:';
const url = new URL(this.request.url, `${protocol}//${this.request.headers.host}`);
const url = new URL(this.request.url, `${protocol}//${this.request.headers.get('host')}`);
Copy link

@kody-ai kody-ai bot Apr 16, 2025

Choose a reason for hiding this comment

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

kody code-review Error Handling high

const host = this.request.headers.get('host');
if (!host) {
    // Option 1: Throw an error if host is required
    throw new Error('Host header is missing');
    // Option 2: Handle appropriately if a fallback isn't possible
    // return API.v1.failure('Missing Host header'); 
}
const url = new URL(this.request.url, `${protocol}//${host}`);

The this.request.headers.get('host') call might return null, leading to potential errors when constructing a URL with an invalid host part.

This issue appears in multiple locations:

  • apps/meteor/app/integrations/server/api/api.js: Lines 97-97
  • apps/meteor/app/integrations/server/api/api.js: Lines 97-97
    Please add validation for the 'host' header to ensure it is present and handle cases where it is missing to prevent errors in URL construction.

Talk to Kody by mentioning @kody

Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.

​

​


const request = {
url: {
Expand Down Expand Up @@ -325,13 +325,22 @@ const middleware = async (c, next) => {
}

try {
const body = Object.fromEntries(new URLSearchParams(await req.raw.clone().text()));
if (!body || typeof body !== 'object' || !('payload' in body) || Object.keys(body).length !== 1) {
const content = await req.raw.clone().text();
const body = Object.fromEntries(new URLSearchParams(content));
if (!body || typeof body !== 'object' || Object.keys(body).length !== 1) {
return next();
}

// need to compose the full payload in this weird way because body-parser thought it was a form
c.set('bodyParams-override', JSON.parse(body.payload));
if (body.payload) {
// need to compose the full payload in this weird way because body-parser thought it was a form
c.set('bodyParams-override', JSON.parse(body.payload));
return next();
}
incomingLogger.debug({
msg: 'Body received as application/x-www-form-urlencoded without the "payload" key, parsed as string',
content,
});
c.set('bodyParams-override', JSON.parse(content));
Copy link

@kody-ai kody-ai bot Apr 16, 2025

Choose a reason for hiding this comment

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

kody code-review Potential Issues high

try {
    const singleKey = Object.keys(body)[0];
    const valueToParse = body[singleKey];
    c.set('bodyParams-override', JSON.parse(valueToParse));
} catch (parseError) {
    incomingLogger.error({ msg: 'Failed to parse form field value as JSON', key: Object.keys(body)[0], error: parseError });
    c.body(JSON.stringify({ success: false, error: 'Invalid JSON in form field value' }), 400);
    return; // Stop processing here
}

Parsing the raw form content (content) as JSON is likely incorrect, as it should be the value of the single form field that is parsed instead.

This issue appears in multiple locations:

  • apps/meteor/app/integrations/server/api/api.js: Lines 343-343
  • apps/meteor/app/integrations/server/api/api.js: Lines 343-343
    Please modify the code to parse the value of the single form field as JSON instead of the raw form content to avoid potential parsing errors.

Talk to Kody by mentioning @kody

Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.

​

​

Comment on lines +336 to +343
Copy link

@kody-ai kody-ai bot Apr 16, 2025

Choose a reason for hiding this comment

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

kody code-review Error Handling medium

try {
    const parsed = JSON.parse(body.payload);
    c.set('bodyParams-override', parsed);
    return next();
} catch (e) {
    incomingLogger.error({ msg: 'Failed to parse payload as JSON', error: e });
    c.body(JSON.stringify({ success: false, error: 'Invalid JSON payload' }), 400);
    return;
}

The JSON.parse calls are within an outer try...catch block, which may lead to generic error handling and less granular error logging.

This issue appears in multiple locations:

  • apps/meteor/app/integrations/server/api/api.js: Lines 336-343
  • apps/meteor/app/integrations/server/api/api.js: Lines 336-343
    Please add specific try...catch blocks around each JSON.parse call to enable more granular error handling and logging.

Talk to Kody by mentioning @kody

Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.

​

​

} catch (e) {
c.body(JSON.stringify({ success: false, error: e.message }), 400);
}
Expand Down
75 changes: 75 additions & 0 deletions apps/meteor/tests/end-to-end/api/incoming-integrations.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -398,6 +398,81 @@ describe('[Incoming Integrations]', () => {

await removeIntegration(withScript._id, 'incoming');
});

it('should send a message if the payload is a application/x-www-form-urlencoded JSON(when not set, default one) but theres no "payload" key, its just a string, the integration has a valid script', async () => {
const payload = { test: 'test' };
let withScript: IIntegration | undefined;

await updatePermission('manage-incoming-integrations', ['admin']);
await request
.post(api('integrations.create'))
.set(credentials)
.send({
type: 'webhook-incoming',
name: 'Incoming test with script and default content-type',
enabled: true,
alias: 'test',
username: 'rocket.cat',
scriptEnabled: true,
overrideDestinationChannelEnabled: false,
channel: '#general',
script:
'const buildMessage = (obj) => {\n' +
' \n' +
' const template = `[#VALUE](${ obj.test })`;\n' +
' \n' +
' return {\n' +
' text: template\n' +
' };\n' +
' };\n' +
' \n' +
' class Script {\n' +
' process_incoming_request({ request }) {\n' +
' msg = buildMessage(request.content);\n' +
' \n' +
' return {\n' +
' content:{\n' +
' text: msg.text\n' +
' }\n' +
' };\n' +
' }\n' +
' }\n' +
' \n',
})
.expect('Content-Type', 'application/json')
.expect(200)
.expect((res) => {
expect(res.body).to.have.property('success', true);
expect(res.body).to.have.property('integration').and.to.be.an('object');
withScript = res.body.integration;
});

if (!withScript) {
throw new Error('Integration not created');
}

await request
.post(`/hooks/${withScript._id}/${withScript.token}`)
.send(JSON.stringify(payload))
.expect(200)
.expect(async () => {
return request
.get(api('channels.messages'))
.set(credentials)
.query({
roomId: 'GENERAL',
})
.expect('Content-Type', 'application/json')
.expect(200)
.expect((res) => {
expect(res.body).to.have.property('success', true);
expect(res.body).to.have.property('messages').and.to.be.an('array');
expect(!!(res.body.messages as IMessage[]).find((m) => m.msg === '[#VALUE](test)')).to.be.true;
});
});

await removeIntegration(withScript._id, 'incoming');
});
});

describe('[/integrations.history]', () => {
Expand Down
Loading