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

Remove the dependency on alicdn.com #187

Closed
jpkrohling opened this issue Feb 9, 2018 · 18 comments · Fixed by #1053 · May be fixed by #516
Closed

Remove the dependency on alicdn.com #187

jpkrohling opened this issue Feb 9, 2018 · 18 comments · Fixed by #1053 · May be fixed by #516
Assignees

Comments

@jpkrohling
Copy link
Contributor

Jaeger might be deployed on a restricted network, so, depending on resources from alicdn.com should be avoided.

image

@yurishkuro
Copy link
Member

@tiffon can we just embed the dependencies?

@tiffon tiffon self-assigned this Feb 13, 2018
@tiffon
Copy link
Member

tiffon commented Feb 15, 2018

I'll check it out.

@tiffon
Copy link
Member

tiffon commented Mar 5, 2018

To remove the CDN: https://github.com/ant-design/antd-init/tree/master/examples/local-iconfont

@bryanhuntesl
Copy link

I strongly concur - every installation is effectively advertising it's control points to that CDN.

@bryanhuntesl
Copy link

bryanhuntesl commented Jun 11, 2019

There is no reference to that path in this repository, also, who publishes the docker images?

To remove the CDN: https://github.com/ant-design/antd-init/tree/master/examples/local-iconfont

@bryanhuntesl
Copy link

It's interesting that an application intended to monitor the core of one's networks is embedding a beacon hosted on a Chinese CDN.

@bryanhuntesl
Copy link

And surprised this hasn't gotten more attention.

@bryanhuntesl
Copy link

@tiffon - any feedback ?

@bryanhuntesl
Copy link

Ah - b7a3e74 - "Use Ant Design instead of Semantic UI" - looks like you're in for a penny - in for a pound with that dependency... Always be auditing..

@bryanhuntesl
Copy link

So the issue has been brought up in ant-design in 2018 - they were not responsive (surprise) and a workaround was suggested - ant-design/ant-design#11063.

Really, I'd have to ask, do you really want to be cooking this into the middle of your network?

@tiffon
Copy link
Member

tiffon commented Jun 12, 2019

@bryanhuntesl You down-voted local-iconfont, which is basically hosting the font locally. Can you elaborate on why that's not a valid solution?

We're also looking into using antd@3.9.0, which switches to SVGs but at the price of bloating the bundle.

@bryanhuntesl
Copy link

Command injection vulnerability in that package's deps. Is it possible to fork ant-design and remove the CSS that loads the fonts from the PRC CDN ?

root@baae98a82d3e:/antd-init# npm install
npm WARN deprecated jade@0.26.3: Jade has been renamed to pug, please install the latest version of pug instead of jade
npm WARN deprecated to-iso-string@0.0.2: to-iso-string has been deprecated, use @segment/to-iso-string instead.
npm WARN deprecated minimatch@0.3.0: Please update to minimatch 3.0.2 or higher to avoid a RegExp DoS issue

> spawn-sync@1.0.15 postinstall /antd-init/node_modules/spawn-sync
> node postinstall

npm notice created a lockfile as package-lock.json. You should commit this file.
added 165 packages from 381 contributors and audited 319 packages in 7.314s
found 4 vulnerabilities (2 low, 1 high, 1 critical)
  run `npm audit fix` to fix them, or `npm audit` for details
root@baae98a82d3e:/antd-init# npm audit  
                                                                                
                       === npm audit security report ===                        
                                                                                
# Run  npm install --save-dev mocha@6.1.4  to resolve 3 vulnerabilities
SEMVER WARNING: Recommended action is a potentially breaking change
                                                                                
  High            Regular Expression Denial of Service                          
                                                                                
  Package         minimatch                                                     
                                                                                
  Dependency of   mocha [dev]                                                   
                                                                                
  Path            mocha > glob > minimatch                                      
                                                                                
  More info       https://npmjs.com/advisories/118                              
                                                                                


                                                                                
  Low             Regular Expression Denial of Service                          
                                                                                
  Package         debug                                                         
                                                                                
  Dependency of   mocha [dev]                                                   
                                                                                
  Path            mocha > debug                                                 
                                                                                
  More info       https://npmjs.com/advisories/534                              
                                                                                


                                                                                
  Critical        Command Injection                                             
                                                                                
  Package         growl                                                         
                                                                                
  Dependency of   mocha [dev]                                                   
                                                                                
  Path            mocha > growl                                                 
                                                                                
  More info       https://npmjs.com/advisories/146                              
                                                                                


# Run  npm install vinyl-fs@3.0.3  to resolve 1 vulnerability
SEMVER WARNING: Recommended action is a potentially breaking change
                                                                                
  Low             Regular Expression Denial of Service                          
                                                                                
  Package         braces                                                        
                                                                                
  Dependency of   vinyl-fs                                                      
                                                                                
  Path            vinyl-fs > glob-stream > micromatch > braces                  
                                                                                
  More info       https://npmjs.com/advisories/786                              
                                                                                


found 4 vulnerabilities (2 low, 1 high, 1 critical) in 319 scanned packages
  4 vulnerabilities require semver-major dependency updates.

https://www.npmjs.com/advisories/146

Overview

Affected versions of growl do not properly sanitize input prior to passing it into a shell command, allowing for arbitrary command execution.
Remediation

Update to version 1.10.2 or later.
Resources

tj/node-growl#60
tj/node-growl#61

@tiffon
Copy link
Member

tiffon commented Jun 13, 2019

@bryanhuntesl I see, thanks.

Rather than forking antd, I think it's easier to serve the fonts (without the local-iconfont package) from /static and add the URL override to our less overrides.

@yurishkuro
Copy link
Member

What exactly does local-iconfont package do? I thought it's built-time only tool that will package fonts as static assets.

@tiffon
Copy link
Member

tiffon commented Jun 13, 2019

The fonts are static assets. We can simply copy them into our public/static folder and serve them out of static. The complication we need to deal with is the path-prefix.

Regarding what local-iconfont does, not really sure; serving the static assets from our public is what I had in mind.

@clippit
Copy link

clippit commented Mar 3, 2020

From https://github.com/ant-design/antd-init/tree/master/examples/local-iconfont:

antd@3.9.0+ 之后图标采用 svg 实现,不再从远程加载字体图标
antd@3.9.0+ and subsequent version uses svg icon and no longer loads icon font from remote

So this issue could be resolved by upgrading current dependency (that is 3.8.0) or removing it completely (#520).

@jpkrohling
Copy link
Contributor Author

@clippit would you like to give it a try?

@clippit
Copy link

clippit commented Mar 3, 2020

@jpkrohling antd@3.8.0 is pretty old but current package.json is sticked to that version since #264. I think we should consider if antd dependency can be removed in near future version by #520. If not, upgrade to at least 3.9.0 may eliminate the annoying CDN fonts, which can be a quick workaround if no other issues occurs. But as @tiffon mentioned, using 3.9.0 will increase bundle size.

yurishkuro added a commit that referenced this issue Nov 14, 2022
Resolves #187

Despite the concerns in the original ticket, it does not seem to affect
the bundle size, because SVG icons are inlined, so the full font file is
not added to the bundle.
```
$ du -h cmd/query/app/ui/actual/
3.8M	cmd/query/app/ui/actual/
```

Signed-off-by: Yuri Shkuro <github@ysh.us>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants