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

Urgent ‼️ 🚨 Issue. #165

Closed
HammadUllahKhan12 opened this issue May 22, 2024 · 9 comments
Closed

Urgent ‼️ 🚨 Issue. #165

HammadUllahKhan12 opened this issue May 22, 2024 · 9 comments

Comments

@HammadUllahKhan12
Copy link

I am using this package in my codebase. As you know that we have ^ in our package.json file with the package version like that "aws4": "^1.12.0",
So it automatically update the package and after that it causing the issue. the header is passing when the sign the request. we need to pass the header manually to fix the issue.
Can we make sure that if someone is using pervious implementation it did not cause the issue.
signer.sign();
will not send the header.

@mhart
Copy link
Owner

mhart commented May 22, 2024 via email

@HammadUllahKhan12
Copy link
Author

yes sure @mhart

` private async request(params: RestParameters): Promise < any > {
const host = this.options.vpc ? this.config.vpc[this.options.env] : this.config.api[this.options.env];

    const q = Object.keys(params.query || {})
        .filter(x => params.query && params.query[x] !== undefined && params.query[x] !== null)
        .reduce((res, x) => {
            res[x] = params.query ? params.query[x] : undefined;
            return res;
        }, {});

    const url = host.path + params.path + (
        Object.keys(q).length > 0 ?
        '?' + querystring.stringify(q, {
            arrayFormat: 'bracket'
        }).replace(/\[\]/g, '%5B%5D') :
        ''
    );

    const request: AxiosRequestConfig = {
        url: 'https://' + host.host + url,
        method: params.method as any,
        headers: {},
        data: params.body,
        adapter: axiosAdapter,
    };

    if (params.method !== 'GET') {
        if (!request.headers) {
            request.headers = {};
        }

        request.headers['Content-Type'] = 'application/json';
    }

    const credentials = await new Promise((resolve, reject) => {
        awscred.loadCredentials((error, credentials) => {
            if (error) {
                return reject(error);
            }

            resolve(credentials);
        });
    });

    let subsegment: AWSXRay.Subsegment | undefined = undefined;

    if (this.options.trace) {
        const segment = AWSXRay.getSegment() as AWSXRay.Segment;

        if (segment && segment.trace_id) {
            subsegment = segment.addNewSubsegment(`S2S ${params.path}`);

            if (!request.headers) {
                request.headers = {};
            }

            request.headers['X-Amzn-Trace-Id'] = `Root=${segment.trace_id};Parent=${subsegment.id};Sampled=${segment['notTraced'] ? 0 : 1}`;
        }
    }

    if (this.options.env !== EClientStage.Production) {
        this.options.logger?.debug({
            url,
            method: params.method,
            trace: request.headers?.['X-Amzn-Trace-Id'],
        }, `API request.`);
    }

    const opts = {
        method: params.method,
        body: params.body ? JSON.stringify(params.body) : undefined,
        path: url,
        headers: request.headers,
        region: this.config.region,
        host: host.host,
        json: true,
        service: 'execute-api',
    };

    const signer = new aws4.RequestSigner(opts, credentials);
    signer.sign();

    let e;

    try {
        const response = await axios(request);

        return response.data;
    } catch (error) {
        const err = e = error.isAxiosError ? error as AxiosError < any > : null;

        if (!err) {
            this.options.logger?.debug({
                url,
                error,
                method: params.method,
            }, `API error.`);

            throw err;
        }

        const status = err.response?.data?.statusCode || parseInt(err.code || '0', 10);

        const trace = err.response?.headers['x-amzn-trace-id'] || err.response?.headers['X-Amzn-Trace-Id'];

        if (status >= 400) {
            const err = new ApiError(
                error.response.data?.message || error.message,
                params.method,
                url,
                status,
                error.response.data,
                trace,
            );

            this.options.logger?.debug({
                url,
                trace,
                error: err,
                method: params.method,
            }, `API error.`);

            throw err;
        }

        this.options.logger?.warn({
            url,
            error,
            trace,
            method: params.method,
        }, `API failure.`);

        throw error;
    } finally {
        if (subsegment) {
            subsegment.close(e);
        }
    }
}

}`
this is function i am using to call the apis by siging the request.

@mhart
Copy link
Owner

mhart commented May 22, 2024 via email

@mhart
Copy link
Owner

mhart commented May 22, 2024

This should work:

    const signer = new aws4.RequestSigner(opts, credentials);
    signer.sign();

    request.headers = opts.headers;

    let e;

    try {
        const response = await axios(request);

It looks like the old code was somehow relying on headers to be the same object – but aws4 modifies this object when it's signing the request.

@mhart
Copy link
Owner

mhart commented May 23, 2024

Also, @HammadUllahKhan12 – was this code copied or inspired from anywhere, or was it just written from scratch?

I'm just trying to figure out how many other people might have been trying to do the same thing where they don't actually use the object they're passing in to sign, but instead are relying on the nested headers object to be the same thing after signing. It's definitely not documented like that in the README or anything – so it was never "supposed" to work, but other people might have accidentally been doing it like you too.

@HammadUllahKhan12
Copy link
Author

@mhart it was written by me 4 years back.

@mhart
Copy link
Owner

mhart commented May 23, 2024

Ok. Well, apologies that this broke your code. It wasn't supposed to ever work like that in the first place, so it was just an accident that it was ever working to begin with. Just reiterating: the right way to use this library is to read properties from opts (or whatever the name is of the request property you pass in to the signer) after you sign it. It seems in this case you were constructing request before you were signing opts, and just accidentally relying on headers being the same object.

Best fix is to just read out from opts anything you need after signing (eg, the headers). Second best fix is just to pin your dependency at version 1.12.0 if you really have to.

@mhart mhart closed this as completed May 23, 2024
@ryanblock
Copy link
Contributor

@HammadUllahKhan12 seems like @mhart's fix is both minor and straightforward to reason about. Alternatively, you can pin your version to 1.12!

@HammadUllahKhan12
Copy link
Author

@ryanblock yeah i already did that. my production server was broke last night so that i already lock the version.

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

3 participants