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

Blocking certain URIs/Patterns from fetch tracking (patch included) #1089

Closed
ewnavilae opened this issue Oct 16, 2019 · 11 comments · Fixed by #1287
Closed

Blocking certain URIs/Patterns from fetch tracking (patch included) #1089

ewnavilae opened this issue Oct 16, 2019 · 11 comments · Fixed by #1287

Comments

@ewnavilae
Copy link

I have recently run into an issue where I had to block AppInsights from sending correlation headers to a specific URI via fetch. I understand I can stop these URIs from being tracked with a plugin that prevents this information from being sent to the telemetry server, however this doesn't stop fetch from being hooked and the headers from being sent. I couldn't find a documented feature that results in this behavior.

To work around it, I have applied the following patch using yarn patch-package:
https://gist.github.com/ewnavilae/07dd5aa58377db6c9ab6df116467b44f

If enough people value this change I may try to submit a pull request actually implementing this correctly. Is there an already implemented, supported way of doing this?

@markwolff
Copy link
Contributor

Does using config.correlationHeaderExcludedDomains solve this problem?

@ewnavilae
Copy link
Author

ewnavilae commented Oct 16, 2019

In my use case, I only need to filter a specific URI from a domain I am otherwise including. I may have tested it wrong but it didn't produce the behavior I required.

@markwolff
Copy link
Contributor

Your patch seems to have similar behavior to the correlationHeaderExcludedDomains. It accepts wildcard *. Can you give an example of your scenario?

@ewnavilae
Copy link
Author

ewnavilae commented Oct 17, 2019

In my AppInsights configuration, I tried the following options:

          correlationHeaderDomains: [
           "my_api_server",
           ],
          correlationHeaderExcludedDomains: [
            "my_api_server/AMCalendarEventsAPI/IcsGenerator.ashx",
           ],

This still results in correlation headers being sent to that URI.
This is problematic because it results in the following error:

Access to fetch at 'my_api_server/AMCalendarEventsAPI/IcsGenerator.ashx' from origin
'http://localhost:8080' has been blocked by CORS policy: Request header field request-id is not
allowed by Access-Control-Allow-Headers in preflight response.

Am I doing something wrong here?

@markwolff
Copy link
Contributor

I believe you need to include the actual hostnames in the 2 config arrays since we do URL parsing on them during the comparison. e.g. localhost:8080/my_api_server and production.endpoint.com/my_api_server. Perhaps they should be updated to support wild carding of the hostname as well */my_api_server

@ewnavilae
Copy link
Author

I'm sorry, I don't understand how that would get correlation headers to be sent to my API server on all URIs except the one I needed to blacklist.

I got this fixed by updating the problematic endpoint to allow the correlation headers.

Still, I believe a better mechanism to choose where to send correlation headers would be useful... Perhaps an option like shouldSendCorrelationHeaders whose signature would be ( url: string ) => boolean? Not sure if it's worth implementing as it would seem this isn't highly requested.

@markwolff
Copy link
Contributor

markwolff commented Oct 23, 2019

See the implementation here. The URLs you configure with must include the hostname, and not just the route information.

public static canIncludeCorrelationHeader(config: ICorrelationConfig, requestUrl: string, currentHost: string) {
if (config && config.disableCorrelationHeaders) {
return false;
}
if (!requestUrl) {
return false;
}
const requestHost = UrlHelper.parseUrl(requestUrl).host.toLowerCase();
if ((!config || !config.enableCorsCorrelation) && requestHost !== currentHost) {
return false;
}
const includedDomains = config && config.correlationHeaderDomains;
if (includedDomains) {
let matchExists;
includedDomains.forEach((domain) => {
const regex = new RegExp(domain.toLowerCase().replace(/\./g, "\.").replace(/\*/g, ".*"));
matchExists = matchExists || regex.test(requestHost);
});
if (!matchExists) {
return false;
}
}
const excludedDomains = config && config.correlationHeaderExcludedDomains;
if (!excludedDomains || excludedDomains.length === 0) {
return true;
}
for (let i = 0; i < excludedDomains.length; i++) {
const regex = new RegExp(excludedDomains[i].toLowerCase().replace(/\./g, "\.").replace(/\*/g, ".*"));
if (regex.test(requestHost)) {
return false;
}
}
return true;
}

@ewnavilae
Copy link
Author

Reading that implementation, it would appear there is no way to exclude a URL inside a host, since only the host is taken into account in the excludedDomains match. The point was to exclude a single URL.

@markwolff
Copy link
Contributor

Yes it seems to only be looking at the hostname and no route info so I'd consider this shortfall a bug/enhancement 😃

@MSNev MSNev added the fixed - waiting release PR Committed and waiting deployment label Jun 30, 2020
@MSNev MSNev mentioned this issue Jun 30, 2020
@MSNev MSNev added this to the 2.5.6 milestone Jul 2, 2020
@MSNev MSNev linked a pull request Jul 2, 2020 that will close this issue
@MSNev MSNev removed the fixed - waiting release PR Committed and waiting deployment label Jul 8, 2020
@MSNev
Copy link
Collaborator

MSNev commented Jul 8, 2020

Release is now fully deployed

@MSNev MSNev closed this as completed Jul 8, 2020
@github-actions
Copy link

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 14, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants