You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@skywalking.apache.org by GitBox <gi...@apache.org> on 2021/03/05 05:11:42 UTC
[GitHub] [skywalking-client-js] tianyk opened a new pull request #45: add `originAllowlist` option
tianyk opened a new pull request #45:
URL: https://github.com/apache/skywalking-client-js/pull/45
Add `originAllowlist` option, only the request origin on this list will be tracked.
```js
import ClientMonitor from 'skywalking-client-js';
ClientMonitor.setPerformance({
collector: 'http://127.0.0.1:8080',
service: 'browser-app',
serviceVersion: '1.0.0',
pagePath: location.href,
useFmp: true,
originAllowlist: ['http://example1.com', /\.example2\.com$/]
});
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-client-js] wu-sheng commented on a change in pull request #45: add `originAllowlist` option
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on a change in pull request #45:
URL: https://github.com/apache/skywalking-client-js/pull/45#discussion_r590391403
##########
File path: README.md
##########
@@ -54,6 +54,7 @@ The register method supports the following parameters.
|vue|Vue|Support vue errors monitoring|false|undefined|
|traceSDKInternal|Boolean|Support tracing SDK internal RPC.|false|false|
|detailMode|Boolean|Support tracing http method and url as tags in spans.|false|true|
+|traceOrigins|string \| RegExp \| (string \| RegExp)[]|Only the request origin on this list will be tracked.|false|-|
Review comment:
And I still prefer `allowList`.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-client-js] tianyk commented on pull request #45: add `originAllowlist` option
Posted by GitBox <gi...@apache.org>.
tianyk commented on pull request #45:
URL: https://github.com/apache/skywalking-client-js/pull/45#issuecomment-791889451
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 <yo...@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 <no...@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
>> <https://github.com/apache/skywalking-client-js/pull/45#issuecomment-791887250>,
>> or unsubscribe
>> <https://github.com/notifications/unsubscribe-auth/AAZ3CNWD6LY4JTS5NXQSPGTTCHID3ANCNFSM4YURSEZQ>
>> .
>>
>
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-client-js] tianyk edited a comment on pull request #45: add `originAllowlist` option
Posted by GitBox <gi...@apache.org>.
tianyk edited a comment on pull request #45:
URL: https://github.com/apache/skywalking-client-js/pull/45#issuecomment-795112518
> > Some examples are Analytics servers, OSS/S3 file upload services.
>
> @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:
1. You cannot be sure which services have been accessed.
2. Only track specific services (for example, only some services use skywalking)
I think origin filtering is necessary, use trace or no trace list.
@wu-sheng @Fine0830
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-client-js] tianyk commented on pull request #45: add `originAllowlist` option
Posted by GitBox <gi...@apache.org>.
tianyk commented on pull request #45:
URL: https://github.com/apache/skywalking-client-js/pull/45#issuecomment-793949480
> @tianyk Hi, you should know, the community and people like us work globally and in async/flexible. So, in the open-source, 3 days is very common to sync and update.
>
> And, right now, I think I am convinced this allowList is better than blockList.
>
> @Fine0830 Please consider this, if you still have concerns, let's discuss them.
I understand. I made the changes as suggested.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-client-js] Fine0830 edited a comment on pull request #45: add `originAllowlist` option
Posted by GitBox <gi...@apache.org>.
Fine0830 edited a comment on pull request #45:
URL: https://github.com/apache/skywalking-client-js/pull/45#issuecomment-794988259
> > 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?
>
> When using JS-SDK, we don't directly have to make our own HTTP requests.
>
> @wu-sheng @Fine0830
>
> ```
> <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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-client-js] kezhenxu94 commented on pull request #45: add `originAllowlist` option
Posted by GitBox <gi...@apache.org>.
kezhenxu94 commented on pull request #45:
URL: https://github.com/apache/skywalking-client-js/pull/45#issuecomment-793326885
> @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:
1. Websites are revolving, new (existing) 3rd-party domains may be added (changed) without advance notice, `blocklist` will cause those requests failed, while `allowlist` may only miss some traces when new controllable domains are added.
2. Adopting `blocklist` and using exclusive match with regex to allowlist some domains is counter-intuitive and error-prone, which is unnecessary if we know users are highly possible to configure in `allowlist` way.
@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, :)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-client-js] tianyk commented on pull request #45: add `originAllowlist` option
Posted by GitBox <gi...@apache.org>.
tianyk commented on pull request #45:
URL: https://github.com/apache/skywalking-client-js/pull/45#issuecomment-791306492
> 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 `window.location.host` as the default value? What do others think?
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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-client-js] tianyk commented on pull request #45: add `originAllowlist` option
Posted by GitBox <gi...@apache.org>.
tianyk commented on pull request #45:
URL: https://github.com/apache/skywalking-client-js/pull/45#issuecomment-791886009
I suggest whitelisting and blocking only the services we care about. Not
all origins.
吴晟 Wu Sheng <no...@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
> <https://github.com/apache/skywalking-client-js/pull/45#issuecomment-791885250>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AAZ3CNSQLL7V762AIUCTLI3TCHGCRANCNFSM4YURSEZQ>
> .
>
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-client-js] Fine0830 commented on pull request #45: add `originAllowlist` option
Posted by GitBox <gi...@apache.org>.
Fine0830 commented on pull request #45:
URL: https://github.com/apache/skywalking-client-js/pull/45#issuecomment-791892922
> 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 <no...@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)](https://github.com/apache/skywalking-client-js/pull/45#issuecomment-791887250)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAZ3CNWD6LY4JTS5NXQSPGTTCHID3ANCNFSM4YURSEZQ> .
skywalking-client-js` is dependent on browser. For the frontend web projects, if you use third-party projects in your own projects, probably, you should use iframe or a router with source codes. You said you didn't know the service address, do you use iframe in your web projects? If so, I have tested, the requests of the third party isn't set `sw8` in the header.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-client-js] kezhenxu94 commented on pull request #45: add `originAllowlist` option
Posted by GitBox <gi...@apache.org>.
kezhenxu94 commented on pull request #45:
URL: https://github.com/apache/skywalking-client-js/pull/45#issuecomment-791886273
> > 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 @kezhenxu94 @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
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-client-js] Fine0830 commented on pull request #45: add `originAllowlist` option
Posted by GitBox <gi...@apache.org>.
Fine0830 commented on pull request #45:
URL: https://github.com/apache/skywalking-client-js/pull/45#issuecomment-797501367
> > > > > You cannot be sure which services have been accessed.
> > > >
> > > >
> > > > This is the key we are arguing right now, as a product env, why this is unknown.
> > >
> > >
> > > @wu-sheng
> > > 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.
> >
> >
> > Do you agree with `blockList`? Could you update the logic to `blockList`?
>
> 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.
>
> @Fine0830 @wu-sheng
I think it's fine, we just use one.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-client-js] wu-sheng commented on pull request #45: add `originAllowlist` option
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on pull request #45:
URL: https://github.com/apache/skywalking-client-js/pull/45#issuecomment-795552012
> You cannot be sure which services have been accessed.
This is the key we are arguing right now, as a product env, why this is unknown.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-client-js] wu-sheng commented on a change in pull request #45: add `originAllowlist` option
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on a change in pull request #45:
URL: https://github.com/apache/skywalking-client-js/pull/45#discussion_r590391139
##########
File path: README.md
##########
@@ -54,6 +54,7 @@ The register method supports the following parameters.
|vue|Vue|Support vue errors monitoring|false|undefined|
|traceSDKInternal|Boolean|Support tracing SDK internal RPC.|false|false|
|detailMode|Boolean|Support tracing http method and url as tags in spans.|false|true|
+|traceOrigins|string \| RegExp \| (string \| RegExp)[]|Only the request origin on this list will be tracked.|false|-|
Review comment:
> Only the request origin on this list will be tracked.
The allow list to determine activating tracing.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-client-js] Fine0830 commented on pull request #45: add `originAllowlist` option
Posted by GitBox <gi...@apache.org>.
Fine0830 commented on pull request #45:
URL: https://github.com/apache/skywalking-client-js/pull/45#issuecomment-794988259
> > 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?
>
> When using JS-SDK, we don't directly have to make our own HTTP requests.
>
> @wu-sheng @Fine0830
>
> ```
> <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 a error with Cross domain.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-client-js] Fine0830 commented on pull request #45: add `originAllowlist` option
Posted by GitBox <gi...@apache.org>.
Fine0830 commented on pull request #45:
URL: https://github.com/apache/skywalking-client-js/pull/45#issuecomment-797189707
> > > You cannot be sure which services have been accessed.
> >
> >
> > This is the key we are arguing right now, as a product env, why this is unknown.
>
> @wu-sheng
>
> 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.
Do you agree with `blockList`? Could you update the logic to `blockList`?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-client-js] wu-sheng commented on pull request #45: add `originAllowlist` option
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on pull request #45:
URL: https://github.com/apache/skywalking-client-js/pull/45#issuecomment-794010712
> how can we solve this problem if we don't know the service address?
Good question, I want to hear about this.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-client-js] wu-sheng commented on pull request #45: add `originAllowlist` option
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on pull request #45:
URL: https://github.com/apache/skywalking-client-js/pull/45#issuecomment-791887250
At least, if you insist you can't, use the regex to do exclusive match.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-client-js] tianyk commented on pull request #45: add `originAllowlist` option
Posted by GitBox <gi...@apache.org>.
tianyk commented on pull request #45:
URL: https://github.com/apache/skywalking-client-js/pull/45#issuecomment-793307955
> > > 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](mailto: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)](https://github.com/apache/skywalking-client-js/pull/45#issuecomment-791887250)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAZ3CNWD6LY4JTS5NXQSPGTTCHID3ANCNFSM4YURSEZQ .
> >
> >
> > `skywalking-client-js` is dependent on browser. For the frontend web projects, if you use third-party projects in your own projects, probably, you should use iframe or a router with source codes. You said you didn't know the service address, do you use iframe in your web projects? If so, I have tested, the requests of the third party isn't set `sw8` in the header.
> > 
>
> ```
> <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?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-client-js] Fine0830 edited a comment on pull request #45: add `originAllowlist` option
Posted by GitBox <gi...@apache.org>.
Fine0830 edited a comment on pull request #45:
URL: https://github.com/apache/skywalking-client-js/pull/45#issuecomment-791892922
> 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 <no...@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)](https://github.com/apache/skywalking-client-js/pull/45#issuecomment-791887250)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAZ3CNWD6LY4JTS5NXQSPGTTCHID3ANCNFSM4YURSEZQ> .
`skywalking-client-js` is dependent on browser. For the frontend web projects, if you use third-party projects in your own projects, probably, you should use iframe or a router with source codes. You said you didn't know the service address, do you use iframe in your web projects? If so, I have tested, the requests of the third party isn't set `sw8` in the header.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-client-js] Fine0830 merged pull request #45: add `noTraceOrigins` option
Posted by GitBox <gi...@apache.org>.
Fine0830 merged pull request #45:
URL: https://github.com/apache/skywalking-client-js/pull/45
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-client-js] tianyk commented on pull request #45: add `originAllowlist` option
Posted by GitBox <gi...@apache.org>.
tianyk commented on pull request #45:
URL: https://github.com/apache/skywalking-client-js/pull/45#issuecomment-791888224
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 <no...@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
> <https://github.com/apache/skywalking-client-js/pull/45#issuecomment-791887250>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AAZ3CNWD6LY4JTS5NXQSPGTTCHID3ANCNFSM4YURSEZQ>
> .
>
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-client-js] Fine0830 commented on pull request #45: add `originAllowlist` option
Posted by GitBox <gi...@apache.org>.
Fine0830 commented on pull request #45:
URL: https://github.com/apache/skywalking-client-js/pull/45#issuecomment-791892682
> 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 <no...@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)](https://github.com/apache/skywalking-client-js/pull/45#issuecomment-791887250)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAZ3CNWD6LY4JTS5NXQSPGTTCHID3ANCNFSM4YURSEZQ> .
`
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-client-js] wu-sheng commented on pull request #45: add `originAllowlist` option
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on pull request #45:
URL: https://github.com/apache/skywalking-client-js/pull/45#issuecomment-795051854
> Some examples are Analytics servers, OSS/S3 file upload services.
@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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-client-js] tianyk commented on pull request #45: add `originAllowlist` option
Posted by GitBox <gi...@apache.org>.
tianyk commented on pull request #45:
URL: https://github.com/apache/skywalking-client-js/pull/45#issuecomment-794993768
> > > 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?
> >
> >
> > When using JS-SDK, we don't directly have to make our own HTTP requests.
> > @wu-sheng @Fine0830
> > ```
> > <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 `Access-Control-Allow-Origin` without handling `Access-Control-Request-Headers`
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-client-js] wu-sheng commented on pull request #45: add `originAllowlist` option
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on pull request #45:
URL: https://github.com/apache/skywalking-client-js/pull/45#issuecomment-791885250
> 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 @kezhenxu94 @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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-client-js] Fine0830 removed a comment on pull request #45: add `originAllowlist` option
Posted by GitBox <gi...@apache.org>.
Fine0830 removed a comment on pull request #45:
URL: https://github.com/apache/skywalking-client-js/pull/45#issuecomment-791892682
> 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 <no...@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)](https://github.com/apache/skywalking-client-js/pull/45#issuecomment-791887250)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAZ3CNWD6LY4JTS5NXQSPGTTCHID3ANCNFSM4YURSEZQ> .
`
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-client-js] kezhenxu94 commented on pull request #45: add `originAllowlist` option
Posted by GitBox <gi...@apache.org>.
kezhenxu94 commented on pull request #45:
URL: https://github.com/apache/skywalking-client-js/pull/45#issuecomment-791311717
> > 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 `window.location.host` as the default value? What do others think?
>
>
>
> 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.
OK, then it looks good to me.
@wu-sheng does👇 make sense to you
> Now, the SDK will intercept all XHR requests and add ws8 request headers. In the cross-domain, the server needs to add headers.
>
> Access-Control-Allow-Headers: sw8
>
> 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.
The point is that some requests to 3rd-party servers cannot add `Access-Control-Allow-Headers: sw8`
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-client-js] tianyk commented on pull request #45: add `originAllowlist` option
Posted by GitBox <gi...@apache.org>.
tianyk commented on pull request #45:
URL: https://github.com/apache/skywalking-client-js/pull/45#issuecomment-794997733
> 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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-client-js] tianyk commented on pull request #45: add `originAllowlist` option
Posted by GitBox <gi...@apache.org>.
tianyk commented on pull request #45:
URL: https://github.com/apache/skywalking-client-js/pull/45#issuecomment-797341667
> > > > You cannot be sure which services have been accessed.
> > >
> > >
> > > This is the key we are arguing right now, as a product env, why this is unknown.
> >
> >
> > @wu-sheng
> > 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.
>
> Do you agree with `blockList`? Could you update the logic to `blockList`?
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.
@Fine0830 @wu-sheng
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-client-js] Fine0830 closed pull request #45: add `originAllowlist` option
Posted by GitBox <gi...@apache.org>.
Fine0830 closed pull request #45:
URL: https://github.com/apache/skywalking-client-js/pull/45
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-client-js] Fine0830 commented on pull request #45: add `originAllowlist` option
Posted by GitBox <gi...@apache.org>.
Fine0830 commented on pull request #45:
URL: https://github.com/apache/skywalking-client-js/pull/45#issuecomment-794994361
BTW, probably the Risk Control Technology Department don't allow us to use external interface, as there are risks.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-client-js] tianyk commented on pull request #45: add `originAllowlist` option
Posted by GitBox <gi...@apache.org>.
tianyk commented on pull request #45:
URL: https://github.com/apache/skywalking-client-js/pull/45#issuecomment-794784650
> 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?
When using JS-SDK, we don't directly have to make our own HTTP requests.
@wu-sheng @Fine0830
```html
<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>
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-client-js] Fine0830 commented on pull request #45: add `originAllowlist` option
Posted by GitBox <gi...@apache.org>.
Fine0830 commented on pull request #45:
URL: https://github.com/apache/skywalking-client-js/pull/45#issuecomment-795031731
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
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-client-js] Fine0830 commented on pull request #45: add `originAllowlist` option
Posted by GitBox <gi...@apache.org>.
Fine0830 commented on pull request #45:
URL: https://github.com/apache/skywalking-client-js/pull/45#issuecomment-791884848
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 @kezhenxu94 @tianyk
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-client-js] wu-sheng commented on pull request #45: add `originAllowlist` option
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on pull request #45:
URL: https://github.com/apache/skywalking-client-js/pull/45#issuecomment-796552726
> > > You cannot be sure which services have been accessed.
> >
> >
> > This is the key we are arguing right now, as a product env, why this is unknown.
>
> @wu-sheng
>
> 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 `blockList`.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-client-js] tianyk commented on pull request #45: add `originAllowlist` option
Posted by GitBox <gi...@apache.org>.
tianyk commented on pull request #45:
URL: https://github.com/apache/skywalking-client-js/pull/45#issuecomment-795112518
> > Some examples are Analytics servers, OSS/S3 file upload services.
>
> @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:
1. You cannot be sure which services have been accessed.
2. Only track specific services (for example, only some services use skywalking)
I think origin filtering is necessary, use trace or no trace list.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-client-js] Fine0830 commented on a change in pull request #45: add `noTraceOrigins` option
Posted by GitBox <gi...@apache.org>.
Fine0830 commented on a change in pull request #45:
URL: https://github.com/apache/skywalking-client-js/pull/45#discussion_r594814426
##########
File path: src/trace/segment.ts
##########
@@ -48,8 +48,8 @@ export default function traceSegment(options: CustomOptionsType) {
url = new URL(window.location.href);
url.pathname = config[1];
}
- if (Array.isArray(options.originAllowList)) {
- const traced = options.originAllowList.some((rule) => {
+ if (Array.isArray(options.noTraceOrigins)) {
Review comment:
Could you add default value(`[]`) for `noTraceOrigins` to remove this `if`.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-client-js] tianyk commented on pull request #45: add `originAllowlist` option
Posted by GitBox <gi...@apache.org>.
tianyk commented on pull request #45:
URL: https://github.com/apache/skywalking-client-js/pull/45#issuecomment-791310681
> 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?
Suppose we want to under the domain name `https://example.com`, now we are going to visit an interface under `https://openapi.example.cn` server.
```
Access-Control-Allow-Headers: ws8
```
Now you have a cross-domain situation that requires the `openapi.example.cn` server to add a response header. If, `openapi.example.cn` is out of our control(third party services). We can't add this response header. If you do not add it, the cross - domain will fail.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-client-js] kezhenxu94 commented on pull request #45: add `originAllowlist` option
Posted by GitBox <gi...@apache.org>.
kezhenxu94 commented on pull request #45:
URL: https://github.com/apache/skywalking-client-js/pull/45#issuecomment-791299826
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 `window.location.host` as the default value? What do others think?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-client-js] tianyk commented on a change in pull request #45: add `noTraceOrigins` option
Posted by GitBox <gi...@apache.org>.
tianyk commented on a change in pull request #45:
URL: https://github.com/apache/skywalking-client-js/pull/45#discussion_r594815452
##########
File path: src/trace/segment.ts
##########
@@ -48,8 +48,8 @@ export default function traceSegment(options: CustomOptionsType) {
url = new URL(window.location.href);
url.pathname = config[1];
}
- if (Array.isArray(options.originAllowList)) {
- const traced = options.originAllowList.some((rule) => {
+ if (Array.isArray(options.noTraceOrigins)) {
Review comment:
👌
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-client-js] kezhenxu94 edited a comment on pull request #45: add `originAllowlist` option
Posted by GitBox <gi...@apache.org>.
kezhenxu94 edited a comment on pull request #45:
URL: https://github.com/apache/skywalking-client-js/pull/45#issuecomment-791886273
> > 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 @kezhenxu94 @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 since you know which domains you own / control
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-client-js] tianyk commented on pull request #45: add `originAllowlist` option
Posted by GitBox <gi...@apache.org>.
tianyk commented on pull request #45:
URL: https://github.com/apache/skywalking-client-js/pull/45#issuecomment-796519526
> > You cannot be sure which services have been accessed.
>
> This is the key we are arguing right now, as a product env, why this is unknown.
@wu-sheng
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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-client-js] wu-sheng commented on a change in pull request #45: add `originAllowlist` option
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on a change in pull request #45:
URL: https://github.com/apache/skywalking-client-js/pull/45#discussion_r590384693
##########
File path: README.md
##########
@@ -54,6 +54,7 @@ The register method supports the following parameters.
|vue|Vue|Support vue errors monitoring|false|undefined|
|traceSDKInternal|Boolean|Support tracing SDK internal RPC.|false|false|
|detailMode|Boolean|Support tracing http method and url as tags in spans.|false|true|
+|traceOrigins|string \| RegExp \| (string \| RegExp)[]|Only the request origin on this list will be tracked.|false|-|
Review comment:
The Type column seems strange to me.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-client-js] wu-sheng commented on pull request #45: add `originAllowlist` option
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on pull request #45:
URL: https://github.com/apache/skywalking-client-js/pull/45#issuecomment-791358492
> > > 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 `window.location.host` as the default value? What do others think?
> >
> >
> > 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.
>
> OK, then it looks good to me.
>
> @wu-sheng does👇 make sense to you
>
> > Now, the SDK will intercept all XHR requests and add ws8 request headers. In the cross-domain, the server needs to add headers.
> > Access-Control-Allow-Headers: sw8
> > 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.
>
> The point is that some requests to 3rd-party servers cannot add `Access-Control-Allow-Headers: sw8`
Make sense.
@Fine0830 @arugal Please do code review.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-client-js] wu-sheng commented on a change in pull request #45: add `originAllowlist` option
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on a change in pull request #45:
URL: https://github.com/apache/skywalking-client-js/pull/45#discussion_r590384693
##########
File path: README.md
##########
@@ -54,6 +54,7 @@ The register method supports the following parameters.
|vue|Vue|Support vue errors monitoring|false|undefined|
|traceSDKInternal|Boolean|Support tracing SDK internal RPC.|false|false|
|detailMode|Boolean|Support tracing http method and url as tags in spans.|false|true|
+|traceOrigins|string \| RegExp \| (string \| RegExp)[]|Only the request origin on this list will be tracked.|false|-|
Review comment:
The Type column seems strange to me. It should be `Array composed by string and regex.`
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-client-js] wu-sheng commented on pull request #45: add `originAllowlist` option
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on pull request #45:
URL: https://github.com/apache/skywalking-client-js/pull/45#issuecomment-791305300
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 automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-client-js] wu-sheng commented on pull request #45: add `originAllowlist` option
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on pull request #45:
URL: https://github.com/apache/skywalking-client-js/pull/45#issuecomment-793942548
@tianyk Hi, you should know, the community and people like us work globally and in async/flexible. So, in the open-source, 3 days is very common to sync and update.
And, right now, I think I am convinced this allowList is better than blockList.
@Fine0830 Please consider this, if you still have concerns, let's discuss them.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-client-js] Fine0830 commented on pull request #45: add `originAllowlist` option
Posted by GitBox <gi...@apache.org>.
Fine0830 commented on pull request #45:
URL: https://github.com/apache/skywalking-client-js/pull/45#issuecomment-793978708
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?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-client-js] wu-sheng commented on pull request #45: add `originAllowlist` option
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on pull request #45:
URL: https://github.com/apache/skywalking-client-js/pull/45#issuecomment-797503652
Only block list makes sense to me.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-client-js] tianyk commented on pull request #45: add `originAllowlist` option
Posted by GitBox <gi...@apache.org>.
tianyk commented on pull request #45:
URL: https://github.com/apache/skywalking-client-js/pull/45#issuecomment-791886759
Zhenxu Ke <no...@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
> <https://github.com/apache/skywalking-client-js/pull/45#issuecomment-791886273>,
> 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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-client-js] tianyk commented on pull request #45: add `originAllowlist` option
Posted by GitBox <gi...@apache.org>.
tianyk commented on pull request #45:
URL: https://github.com/apache/skywalking-client-js/pull/45#issuecomment-791895318
> > 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](mailto: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)](https://github.com/apache/skywalking-client-js/pull/45#issuecomment-791887250)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAZ3CNWD6LY4JTS5NXQSPGTTCHID3ANCNFSM4YURSEZQ .
>
> `skywalking-client-js` is dependent on browser. For the frontend web projects, if you use third-party projects in your own projects, probably, you should use iframe or a router with source codes. You said you didn't know the service address, do you use iframe in your web projects? If so, I have tested, the requests of the third party isn't set `sw8` in the header.
>
> 
```html
<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>
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-client-js] wu-sheng commented on pull request #45: add `originAllowlist` option
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on pull request #45:
URL: https://github.com/apache/skywalking-client-js/pull/45#issuecomment-791887042
Why you don't? From my understanding, even you don't know now, you should.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org