Conversation
|
I'm personally +1 to this new option, but wondering what's the reasonable default value to give it, now you didn't give any default value so all requests are not intercepted, right? Maybe set |
|
I am wondering the case, when do we use this? Such as which domain should we exclude. Usually this agent is being globally set by an app, then why some of them(especially domain name, rather than URI) should not be traced? |
This is an optional configuration, and it is backward compatible. If it's not configured, it's the same as now. All requests will be traced. |
Suppose we want to under the domain name Now you have a cross-domain situation that requires the |
OK, then it looks good to me. @wu-sheng does👇 make sense to you
The point is that some requests to 3rd-party servers cannot add |
Make sense. @Fine0830 @arugal Please do code review. |
|
There are some suggestions for this. 1. For the SDK name, could you change |
Use blockList if you want. I am +1 to use blocking rather than allowing mechanism. In this case, empty meaning all available makes more sense to me. |
|
I suggest whitelisting and blocking only the services we care about. Not
all origins.
吴晟 Wu Sheng <notifications@github.com>于2021年3月6日 周六14:50写道：
… There are some suggestions for this. 1. For the SDK name, could you change originAllowlist
to NoTraceOrigins and update the corresponding logic? 2. It is fine that
the type of originAllowlist is (string | RegExp)[]. 3. the default value
of originAllowlist is [].
WDYT? @wu-sheng <https://github.com/wu-sheng> @kezhenxu94
<https://github.com/kezhenxu94> @tianyk <https://github.com/tianyk>
Use blockList if you want. I am +1 to use blocking rather than allowing
mechanism. In this case, empty meaning all available makes more sense to me.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#45 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAZ3CNSQLL7V762AIUCTLI3TCHGCRANCNFSM4YURSEZQ>
.
|
Blocking mechanism requires that you must list all uncontrollable domains, some of which may be unknown, allowlist is more preferable in this case since you know which domains you own / control |
|
Zhenxu Ke <notifications@github.com>于2021年3月6日 周六15:00写道：
There are some suggestions for this. 1. For the SDK name, could you change originAllowlist
to NoTraceOrigins and update the corresponding logic? 2. It is fine that
the type of originAllowlist is (string | RegExp)[]. 3. the default value
of originAllowlist is [].
WDYT? @wu-sheng <https://github.com/wu-sheng> @kezhenxu94
<https://github.com/kezhenxu94> @tianyk <https://github.com/tianyk>
Use blockList if you want. I am +1 to use blocking rather than allowing
mechanism. In this case, empty meaning all available makes more sense to me.
Blocking mechanism requires that you must list all uncontrollable domains,
some of which may be unknown, allowlist is more preferable in this case
sonar you know which domains you own / control
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#45 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAZ3CNRCFUY4RI547WKVXH3TCHHHBANCNFSM4YURSEZQ>
.
That's what I mean. Sometimes we don't know all of origins, especially when
using third party services.
|
|
Why you don't? From my understanding, even you don't know now, you should. |
|
At least, if you insist you can't, use the regex to do exclusive match. |
|
Let me give you an example. We used an SDK for a local service, and now we
can know the origin of the service they provide. However, the SDK may be
upgraded and they may change the service address. The situation is out of
your control. For example, an SDK in the form of Google Analytics(Just an
example, Google Analytics does not use XHR).
In addition, adding extra headers can be dangerous when crossing domains.
Services (third) that are not processed across domains will not be
available. To make the service more manageable, I suggest whitelisting.
吴晟 Wu Sheng <notifications@github.com>于2021年3月6日 周六15:07写道：
… At least, if you insist you can't, use the regex to do exclusive match.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#45 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAZ3CNWD6LY4JTS5NXQSPGTTCHID3ANCNFSM4YURSEZQ>
.
|
|
Adding additional headers can be dangerous across domains, making the
service unusable. The blacklist will have uncontrollable situation, the
white list does not have this problem. There can be no uncontrollable
situations in the online service.
https://developer.mozilla.org/en-US/docs/Web/HTTP/CORS
Yongke Tian <yongketian@gmail.com>于2021年3月6日 周六15:16写道：
… Let me give you an example. We used an SDK for a local service, and now we
can know the origin of the service they provide. However, the SDK may be
upgraded and they may change the service address. The situation is out of
your control. For example, an SDK in the form of Google Analytics(Just an
example, Google Analytics does not use XHR).
In addition, adding extra headers can be dangerous when crossing domains.
Services (third) that are not processed across domains will not be
available. To make the service more manageable, I suggest whitelisting.
吴晟 Wu Sheng ***@***.***>于2021年3月6日 周六15:07写道：
> At least, if you insist you can't, use the regex to do exclusive match.
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <#45 (comment)>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AAZ3CNWD6LY4JTS5NXQSPGTTCHID3ANCNFSM4YURSEZQ>
> .
>
|
|
<script src="https://cdn.jsdelivr.net/npm/skywalking-client-js@0.4.0/lib/index.js"></script>
<script>
// Comment out and try it here
ClientMonitor.register({
collector: 'http://127.0.0.1:8080',
service: 'test-ui',
pagePath: '/current/page/name',
serviceVersion: 'v1.0.0'
});
fetch('http://kekek.cc/static/bear.bin');
</script> |
@kezhenxu94 @wu-sheng @Fine0830 Is there any progress on this patch? |
Sorry that this PR hasn't been merged for a little long because it seems that we haven't yet reached to a consensus whether we should allowlist / blocklist. To reiterate, I'm +1 to allowlisting, the critical reason is that:
@tianyk please give the community a little more time to evaluate and reach to a consensus (hopefully) so that we don't make something unreasonable for the users, :) |
I understand. I made the changes as suggested. |
|
I'm curious if there is no problem for the front end to call the third-party interface directly. What's more, in the case of unclear services, such as cross domain, how can we solve this problem if we don't know the service address? |
Good question, I want to hear about this. |
When using JS-SDK, we don't directly have to make our own HTTP requests. <script src="https://cdn.jsdelivr.net/npm/skywalking-client-js@0.4.0/lib/index.js"></script>
<script>
// Comment out and try it here
ClientMonitor.register({
collector: 'http://127.0.0.1:8080',
service: 'test-ui',
pagePath: '/current/page/name',
serviceVersion: 'v1.0.0'
});
</script>
<script>
// js-sdk
(function() {
fetch('http://kekek.cc/static/bear.bin');
})();
</script> |
When I use like this, here is an error with Cross domain. |
You turn off trace, the error is gone. Because in this example, the server only add |
|
BTW, probably the Risk Control Technology Department don't allow us to use external interface, as there are risks. |
Sometimes it's a third party service, but that's still the case. Some examples are Analytics servers, OSS/S3 file upload services. |
|
From my development experience, I usually call such an interface on the back end instead directly calling. That's fine, if you think the example reasonable. @wu-sheng |
@tianyk I want to get more explanation about, why your codes are allowed to randomly accessing different domains without control? I can only see you are using HTTP, rather than HTTPS, which should not be in product env. |
I thought about it, if you are very clear about the service to be called, it is more appropriate to use denylist. In some cases, allowlist may be more appropriate:
I think origin filtering is necessary, use trace or no trace list. |
This is the key we are arguing right now, as a product env, why this is unknown. |
This is not the original problem. My initial requirement was to filter out some requests without tracking. Our situation at the time was that we knew those domain names we needed to track, so we wanted to use a whitelist to solve it. This brings up the above series of discussions on the whitelist. My problem is not actually a whitelist or a blacklist, but the need to filter out some requests. Sorry, some of the above examples have caused misunderstandings. We should go back to filter tracking, I think this is the most fundamental problem. As for the whitelist or the blacklist, it is just a common filtering method. |
Like I replied, I am supporting this feature. But also agree with @Fine0830, it seems blockList makes more sense. From the product management requirement, I prefer |
Do you agree with |
I agree there is a BlockList, shall we consider providing both BlockList and Whitelist configuration? Whitelist can be used for most situations that need to be excluded. |
I think it's fine, we just use one. |
|
Only block list makes sense to me. |
src/trace/segment.ts
Outdated
| } | ||
| if (Array.isArray(options.originAllowList)) { | ||
| const traced = options.originAllowList.some((rule) => { | ||
| if (Array.isArray(options.noTraceOrigins)) { |
There was a problem hiding this comment.
Could you add default value([]) for noTraceOrigins to remove this if.
想请问一个问题，是否允许自定义请求头名呢？如Access-Control-Allow-Headers: xxx-sw8 |
5 participants
Now, the SDK will intercept all
XHRrequests and addws8request headers. In the cross-domain, the server needs to add headers.https://developer.mozilla.org/en-US/docs/Web/HTTP/CORS#preflighted_requests
In some cases, we may request third party services. If a third party server does not add the top header, there will be cross-domain problems.
@wu-sheng
Add
originAllowlistoption, only the request origin on this list will be tracked.