-
Notifications
You must be signed in to change notification settings - Fork 2
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
RK-20569 - CORS rework for Dynatrace apps #387
Conversation
Github Enforcer opened Task: RK-20569 |
return; | ||
} | ||
|
||
const response = await fetch(`${origin}/platform-reserved/dob/isapprefallowed?appOrigin=${origin}`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's add an env variable to skip the HTTP check
@ElDuderinos
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, let's cache the result for a while :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The results are cached (if successful), take a look at line 37.
I didn't want to cache failed results because it might be because of a network issue or timeout or something so I didn't want to prevent it from retrying.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@LiranLast @ElDuderinos As a packaged Electron app can't read system env variables during runtime, I added an option in the debug menu (when you right click the X) to toggle skipping verification. Should I change the text to something more clear?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ron-rookout It may be possible to read env using a package, I remember running into it while patching lang servers
Believe it is called electron-env
Also note this problem mainly occurs in macOs if I recall correctly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Urook I didn't find this one specifically, I have searched for other ways and couldn't find any either
@ron-rookout do we have a solution for the EOL of the legacy app? |
import * as express from "express"; | ||
import { readFileSync } from "fs"; | ||
import { applyMiddleware } from "graphql-middleware"; | ||
import * as http from "http"; | ||
import { join } from "path"; | ||
import { resolvers } from "./api"; | ||
import {getCorsMiddleware} from "./cors"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
import {getCorsMiddleware} from "./cors"; | |
import { getCorsMiddleware } from "./cors"; |
return; | ||
} | ||
|
||
const response = await fetch(`${origin}/platform-reserved/dob/isapprefallowed?appOrigin=${origin}`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ron-rookout It may be possible to read env using a package, I remember running into it while patching lang servers
Believe it is called electron-env
Also note this problem mainly occurs in macOs if I recall correctly
Irrelevant to this repo, merged in the new one, closing |
This PR reworks how CORS is handled for Dynatrace apps.
If request is coming from a valid Dynatrace URL, it will check against the origin's environment facade that the Dynatrace app ref is allowed to access the Desktop app.