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

Bug: Pre-request script is executed before the request is prepared #2249

Closed
2 tasks done
pietrygamat opened this issue May 6, 2024 · 13 comments
Closed
2 tasks done
Labels
bug Something isn't working

Comments

@pietrygamat
Copy link
Contributor

I have checked the following:

  • I use the newest version of bruno.
  • I've searched existing issues and found nothing related to my issue.

Describe the bug

When executing the request, bruno goes through several stages of configuration, among them: execution o pre-request script, post-request script, and run of 'configureRequest` method. The current execution order is:

  • execute pre-request scripts
  • configure request (interpolate vars, sets proxy, auth headers. cookies)
  • execute request
  • execute post-request

I wonder if it would make more sense to execute the steps in this order:

  • configure request
  • execute-pre-request script
  • execute request
  • execute post-request script

This way any logic built into bruno may be overwritten by user's script and on top of that, user's script execute on request that is closer to what is actually being sent (e.g. with variables resolved, headers set).

For example, the below bru file logs this to console:

Before
{{api_url}}
{"Content-Type":"application/json"}

After
https://localhost:8083
{"Content-Type":"application/json","Authorization":"Basic dXNlcjpwYXNzd29yZA=="}

while I would expect the output Before and After to be the same.

.bru file to reproduce the bug

meta {
name: example
type: http
}

post {
url: {{api_url}}
body: text
auth: basic
}

headers {
Content-Type: application/json
}

auth:basic {
username: user
password: password
}

script:pre-request {
console.log('Before');
console.log(req.url);
console.log(JSON.stringify(req.getHeaders()));
}

script:post-response {
console.log('After');
console.log(req.url);
console.log(JSON.stringify(req.getHeaders()));
}

Screenshots/Live demo link

N/A

@pietrygamat pietrygamat added the bug Something isn't working label May 6, 2024
pietrygamat pushed a commit to pietrygamat/bruno that referenced this issue May 6, 2024
The pre-request script phase is now executed after configureRequest. That means the
'req' object in pre-request script has access to more data, including evaluated variables, set authorization headers, etc.

fixes usebruno#2249
@lohxt1
Copy link
Collaborator

lohxt1 commented May 6, 2024

Hey @pietrygamat

We have a fix in place for this, which is planned to be included in the upcoming release.

@centur
Copy link

centur commented May 14, 2024

Wow :(
This bug broke heaps of my existing scripts, I've to find a older bruno version and get back to it, as I'm suddenly blocked .
@lohxt1 I wonder if these types of scripts still possible after the fix.

console.log(`createTenant`)
const { readFileSync } = require('fs');
const { URL } = require('url');
const path = require('path');

var attachmentFilename ="../../my-config.zip"
const attachmentPath = path.join(bru.cwd(), attachmentFilename);

console.log(attachmentPath);

const attachment = readFileSync(attachmentPath);
const attachmentLength = attachment.length;

req.setHeader("Content-Type", "application/zip");
req.setHeader("Content-Length", attachmentLength);
req.setBody(attachment);

var randomName = faker.location.city()
bru.setVar("configName", `bruno-${randomName}`);

For the context - because bruno does not support raw file (and I want some extra control over file selection) I've implemented this script to send raw binary content in post request. few versions ago it was fully functional script, now it's not working at all. Not sure what exactly was broken but errors or console logs from pre-req are not delivered anywhere and this file is not being sent to api eiter
And FYI - console.log in pre-request ceased to work, nothing shows in console even if request is trivial

Bruno version: 1.17.0
MacOS: 14.4.1 (23E224)

@pietrygamat
Copy link
Contributor Author

@lohxt1 do you have an update on this one, or a public branch where this can be peeked at?

We have a fix in place for this, which is planned to be included in the upcoming release.

@centur
Copy link

centur commented May 23, 2024

@pietrygamat check if it's fixed in 1.18 which is just dropped in release channel. It's fixed my scripts with file uploads for sure.

@lohxt1
Copy link
Collaborator

lohxt1 commented May 23, 2024

@pietrygamat

The current behavior in v1.18.0, is that all the interpolation happens at the end of pre-request script execution.

Issue with configuring request before pre-request script :
If the interpolation of the URL happens before the pre-request script, and if the URL is using a variable that is supposed to change in the pre-request script, the new updated variable value won't be replaced in the already interpolated URL.

I was wondering if introducing a new bru function called bru.interpolateVars() help in this case ?
(name's not decided, open for suggestions)

Scenario:
https://www.example.com?x={{foo}}

foo is a var with value bar

console.log('before', req.getURL());

bru.setVar('foo', 'not-bar');
bru.interpolateVars();

console.log('after', req.getURL());
Output:
before https://www.example.com?x={{foo}}
after https://www.example.com?x=not-bar

Let me knwo what you think of this

@lohxt1
Copy link
Collaborator

lohxt1 commented May 23, 2024

@centur

Could you verify whether your issue was resolved with version 1.18.0?

@pietrygamat
Copy link
Contributor Author

@lohxt1

Issue with configuring request before pre-request script : If the interpolation of the URL happens before the pre-request script, and if the URL is using a variable that is supposed to change in the pre-request script, the new updated variable value won't be replaced in the already interpolated URL.

This is a good point. I have been thinking under different assumptions: the script would be able to replace variables on those variables that have not been already replaced (no value provided in env file, etc), but I can see how this is not always the case. In that context proposed solution seems like a reasonable nice-to-have.

That, however, does not solve my core issue here: the req accessed during pre-request script is not processed by configure method, which does a lot more than interpolation - most importantly, sets various fields related to authorization. That is something I hoped to rely on when implementing #2061 - my idea is that obtaining an access token should happen without overwriting user's request - during request configuration - as obtaining access token is functionally the same as for example setting basic auth header, which is also done in configure phase.

So having a script execute on a fully prepared request would allow accessing anything that the general configuration sets for the user (including access_token) and would also theoretically prevent edge cases, where something set in the pre-request script is overwritten by default bruno behavior. But it is a sort of a chicken-egg problem, because one may claim they don't want to read anything set in configuration, but rather feed configure with required data... both approaches have their merits.

If you vote to stay with current behavior, this may be closed.

@centur
Copy link

centur commented May 24, 2024

@lohxt1 yes it does, thank you. I tested it when I saw 1.18 was pushed to brew.sh
#2249 (comment)

Re bru.interpolateVars(); - From the name it's not clear what would it do and what would happens if someone would call it twice (in a collection wide and request-specific pre-request section) - and then dev will be surprised with a result.

With our samples above - it's clear that there are some scenarios when you want to change vars before interpolation (like inferring a full url or adjusting the path like in @pietrygamat scenario) and some cases where we want to have a configured request and re-write parts of it (my file uploads workaround). Taking a step back from solution here I think end result should satisfy both behaviours:

  • Pre-request script should be able to influence interpolation results (so it runs BEFORE final interpolation)
  • Pre-request script should be able to change aspects of request before it's sent.

As an idea - sequence of steps can be something like this:

  1. Request instance is initialized, but not yet interpolated or validated, it's available as req. in script.
    • It can be a shallow object which looks like Request, but I'd prefer OG request and take the risks of me messing it up for the flexibility it gives.
  2. Pre-request script runs, can make changes to env vars and request data itself AND allows a way to omit final interpolation (bru.skipInterpolation(req) - calling this implies developer takes responsibility that they handled interpolation of all values and they don't want those to be changed ).
  3. Request is finalized - all values are interpolated, it's validated, proxy set etc
  4. Request sent.

So it's the sequence that is currently in place with an ability to force skip automatic interpolation, but not proxy\cookies settings. Thoughts ?
UPD: technically this can be fine-tuned and would allow to mark sections of request to skip interpolation, not a full request. Or if you think interpolateVars is a preffered name - do partial in-script interpolation ( as in bru.interpolateVars(req.url)) which would then change nothing at the final interpolation step.

@lohxt1
Copy link
Collaborator

lohxt1 commented May 24, 2024

@pietrygamat

I see that you have another PR (#2077) which supersedes #2061?

Please correct me if I'm wrong, but going through the PR, I see why you'd prefer the request configuration to happen before the pre-request script. When the access_token is obtained through either the authorization process or from the oauth2Store (cached), it gets set to the request object in the configuration step (which takes place after the pre-request execution)
request.credentials = credentials;

Because of this, req.credentials is undefined in the pre-request script but has the correct values in the post-response script. I'm assuming that you implemented this solution due to the absence of dedicated variables for storing the access_token value.

If we had dedicated global variables to store these special tokens, we could use them instead of the oauth2Store as in the current implementation. This would mean that you'd no more need request.credentials, and you'd be okay with the current steps ? i.e

pre-request script execution
request configuration
post-request script execution

I remember having a discussion around dedicated variables in one of the older threads.

A new discussion has been initiated by Anoop regarding the variables in Bruno: #2332

There is a plan to add a new set of variables. Feel free to share your thoughts in this discussion.

@lohxt1
Copy link
Collaborator

lohxt1 commented May 24, 2024

Pre-request script runs, can make changes to env vars and request data itself AND allows a way to omit final interpolation (bru.skipInterpolation(req) - calling this implies developer takes responsibility that they handled interpolation of all values and they don't want those to be changed ).

@centur one scenario that I can think of where bru.interpolateVars() would be sort of useful ?

Before:
interpolated value of req.getURL() is not available in the pre-request-script

329953824-92890840-16c2-4505-8771-7a9a459a302b-2

After:
the bru.substituteVariables() function call will interpolate the values for further usage in the script.
req.getURL() has the interpolated host value

329954074-5fc658a3-f32e-4f2d-a9e9-965bd346f644

In the screenshot the function is called as bru.substituteVars()

Pre-request script should be able to influence interpolation results (so it runs BEFORE final interpolation)

currently you can directly access the variable values, modify them and assign them to parts of request object (req.setHeader, req.setBody) in the pre-request script, which otherwise would have been interpolated at the end of pre-request script execution if untouched.

For example:

In headers:

Bearer {{bearer_token}}

In pre-request script:

const bearer_token = bru.getVar("bearer_token");
req.setHeader('Authentication', `Bearer ${bearer_token}`);

i'm not entirely sure at the moment whether we'd be needing bru.skipInterpolation?
the problem is that things like req URL are not being interpolated in the pre-request script, and hence the proposed bru.interpolateVars function where users can interpolate if they'd like to base their pre-req script logic on the interpolated value.

@pietrygamat
Copy link
Contributor Author

@lohxt1

I see that you have another PR (#2077) which supersedes #2061?

That's right. It may be easier to manage them separately - to sort backend logic in #2061 and required UI changes in #2077 (which I will rebase as required) - or they may be considered a monolitic whole, in which case we can close the older one.

I'm assuming that you implemented this solution due to the absence of dedicated variables for storing the access_token value.

It's not just that. I like the idea of reusing the session cache also for tokens, because they are stored together and cleaned together easily with the same button, and also survive the application restart without being committed to bru files. But that's a discussion better moved to #2061 or #2077 when you get to it.

If we had dedicated global variables to store these special tokens, we could use them instead of the oauth2Store as in the current implementation. This would mean that you'd no more need request.credentials, and you'd be okay with the current steps ?

Depends on the steps - the variables still need to be set somewhere. If we manually click the get access token button, the token request is executed outside of the flow, the variables are set and are available. If however we want to rely on bruno to obtain the token on request run (for example when running from cli), the variables are not present until the configure stage is reached. It's the same case for any other value set in configure stage. Pre-request script is unable to:

Having power to do so seems nice, no?

@lohxt1
Copy link
Collaborator

lohxt1 commented May 25, 2024

oh i'm totally onboard with the idea of using the session cache, this was the same thought process behind using oauth2Store to manage the auth window sessions :)

we could use them instead of the oauth2Store

I meant to say
"use them 'along with' the oauth2store"

And I was mainly talking about how the cache values are set to request.credentials or are set to request.headers, and there needs to be a way to make them accessible in the pre-request script via variables that are not written to the bru files (something like collection variables). In the scenario where token_type is not bearer and the user would need to manually set the session tokens to the request in some other way that's not supported by Bruno currently.



overwrite Authorization token type

this can be done

overwrite_header.mov

An option to append a certificate per request can be achieved by adding a new additional input field that can be set to a variable like '{{caFilePath}}' along with the existing filepicker and can be modified in the pre-request script, and the option to use variables for proxy input fields is already present.

I also think that the way you are fetching the token as part of the configure step in #2077 makes more sense.

(keeping the need for having dedicated variables out of the current discussion)
The way I would go about using this is
If the token_type is bearer, then everything works just fine, the way it is (request headers are set with the obtained token values in the configure step). which covers the majority of the cases. I can use it at both collection level and request level.
But if the token type is something other than 'bearer', then
I would go with collection level auth, then add
bru.setVar('req.credentails', req.credentials); in the collection post-response script.

In the request-level pre-request script, I would then access 'req.credentials' var and modify the request headers/body/url as per my needs to pass the token value. (this type of usage is rare?)

the pros of having pre-request script execution before the configure-request step outweigh the cons i think

@pietrygamat
Copy link
Contributor Author

The hints look like they should work. Let's close this one then.

@pietrygamat pietrygamat closed this as not planned Won't fix, can't repro, duplicate, stale May 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants