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.

Sign up for GitHub

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

Jump to bottom

feat: support unpkg features #456

Merged
merged 6 commits into from
May 5, 2023
Merged

feat: support unpkg features #456

merged 6 commits into from
May 5, 2023

Conversation

fengmk2
Copy link
Member

@fengmk2 fengmk2 commented May 4, 2023 •
edited
Loading

WARN: include sql change

😄 Follow unpkg router
😄 Auto sync files after package version add

closes #452

@fengmk2 fengmk2 added the enhancement New feature or request label May 4, 2023
@socket-security
Copy link

socket-security bot commented May 4, 2023 •
edited
Loading

No dependency changes detected. Learn more about Socket for GitHub ↗︎

👍 No new dependency issues detected in pull request

Bot Commands

To ignore an alert, reply with a comment starting with @SocketSecurity ignore followed by a space separated list of package-name@version specifiers. e.g. @SocketSecurity ignore foo@1.0.0 bar@* or ignore all packages with @SocketSecurity ignore-all

Pull request alert summary
Issue Status
Install scripts ✅ 0 issues
Native code ✅ 0 issues
Bin script shell injection ✅ 0 issues
Unresolved require ✅ 0 issues
Invalid package.json ✅ 0 issues
HTTP dependency ✅ 0 issues
Git dependency ✅ 0 issues
Potential typo squat ✅ 0 issues
Known Malware ✅ 0 issues
Telemetry ✅ 0 issues
Protestware/Troll package ✅ 0 issues

@fengmk2 fengmk2 marked this pull request as ready for review May 4, 2023 17:25
@fengmk2 fengmk2 requested a review from elrrrrrrr May 4, 2023 17:25
@codecov
Copy link

codecov bot commented May 4, 2023 •
edited
Loading

Codecov Report

Merging #456 (e354ab9) into master (dd7d73e) will increase coverage by 0.06%.
The diff coverage is 99.52%.

@@            Coverage Diff             @@
##           master     #456      +/-   ##
==========================================
+ Coverage   97.45%   97.51%   +0.06%     
==========================================
  Files         160      166       +6     
  Lines       14780    15360     +580     
  Branches     1891     1993     +102     
==========================================
+ Hits        14404    14979     +575     
- Misses        376      381       +5
Impacted Files Coverage Δ
app/port/controller/RegistryController.ts 100.00% <ø> (ø)
...pp/port/controller/PackageVersionFileController.ts 99.02% <99.02%> (ø)
app/core/service/PackageVersionFileService.ts 99.23% <99.23%> (ø)
app/common/FileUtil.ts 100.00% <100.00%> (ø)
app/common/PackageUtil.ts 99.00% <100.00%> (+0.01%) ⬆️
app/core/entity/Package.ts 100.00% <100.00%> (ø)
app/core/entity/PackageVersionFile.ts 100.00% <100.00%> (ø)
app/core/event/SyncPackageVersionFile.ts 100.00% <100.00%> (ø)
app/core/service/PackageManagerService.ts 98.93% <100.00%> (+<0.01%) ⬆️
app/core/util/EntityUtil.ts 100.00% <100.00%> (ø)
... and 9 more

... and 2 files with indirect coverage changes

@fengmk2 fengmk2 marked this pull request as draft May 5, 2023 03:07
@elrrrrrrr
Copy link
Member

unpkg 相关接口只用于产物预览吗？如果配置了 contentType， 是否需要处理 CDN 盗链的情况

另外我们是否需要加上开关配置，只对白名单内的包支持预览能力，减少内容安全风险。

@fengmk2 fengmk2 marked this pull request as ready for review May 5, 2023 07:39
@fengmk2
Copy link
Member Author

fengmk2 commented May 5, 2023

unpkg 相关接口只用于产物预览吗？如果配置了 contentType， 是否需要处理 CDN 盗链的情况

另外我们是否需要加上开关配置，只对白名单内的包支持预览能力，减少内容安全风险。

内容问题通过内容扫描服务来解决。我加个开关，默认开启。

@fengmk2
feat: support unpkg features
be613e7 
WARN: include sql change

closes #452
@fengmk2 fengmk2 marked this pull request as draft May 5, 2023 09:55
@fengmk2 fengmk2 marked this pull request as ready for review May 5, 2023 14:43
@fengmk2
Copy link
Member Author

fengmk2 commented May 5, 2023

还差一个主动同步，在 package add event 里面加上。

popomore
popomore reviewed May 5, 2023
View reviewed changes
app/port/controller/PackageVersionFileController.ts
ctx.vary(this.config.cnpmcore.cdnVaryHeader);
const [ scope, name ] = getScopeAndName(fullname);
const packageVersion = await this.#getPackageVersion(ctx, fullname, scope, name, versionOrTag);
ctx.set('cache-control', META_CACHE_CONTROL);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这些感觉应该封装一下

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

headers 设置？

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

应该是所有的，把 http 协议相关的拆分开。比如提供 HttpRequest 和 HttpResponse 对象注入，其实 koa 本身也是分开的，只是都注入到 ctx 了。

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@killagu 又来了，看看怎么搞。

@fengmk2 fengmk2 merged commit 8ec081a into master May 5, 2023
@fengmk2 fengmk2 deleted the unpkg branch May 5, 2023 15:22
fengmk2 pushed a commit that referenced this pull request May 5, 2023
@semantic-release-bot
Release 3.19.0
5f9a7a8 
[skip ci]

## [3.19.0](v3.18.0...v3.19.0) (2023-05-05)

### Features

* support unpkg features ([#456](#456)) ([8ec081a](8ec081a))
@github-actions
Copy link

github-actions bot commented May 5, 2023

🎉 This PR is included in version 3.19.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@baxtergu
Copy link

baxtergu commented May 6, 2023

所有文件信息都存在 package_version_files 这张表会不会有性能问题？

@fengmk2
Copy link
Member Author

fengmk2 commented May 6, 2023

@baxtergu 用目前云上的数据库，单表没有上限。

@baxtergu
Copy link

baxtergu commented May 8, 2023

@baxtergu 用目前云上的数据库，单表没有上限。

https://help.aliyun.com/document_detail/41708.html 是用的 RDS for MySQL 么？

@fengmk2
Copy link
Member Author

fengmk2 commented May 8, 2023 •
edited
Loading

@baxtergu 是的，目前单表早就超过这个限制了。

@baxtergu
Copy link

baxtergu commented May 9, 2023

@fengmk2 了解了，感谢解答。

atian25
atian25 reviewed Jun 1, 2023
View reviewed changes
config/config.default.ts
@@ -87,6 +87,8 @@ export default (appInfo: EggAppConfig) => {
syncNotFound: false,
// redirect to source registry when package not found
redirectNotFound: true,
// enable unpkg features, https://github.com/cnpm/cnpmcore/issues/452
enableUnpkg: true,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fengmk2 这块是不是还需要进一步支持下黑白名单机制，用在 cnpmcore 内网部署时，避免安全合规问题

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

黑名单现在是服用的，blocks 的都不会被 unpkg 解析。
白名单太多，三方库没有特别好的思路。

Copy link
Contributor

@atian25 atian25 Jun 1, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

指的是企业内网部署的场景，一个包要发布到 unpkg，需要审核，避免一些包被无意识的情况下泄漏出去，有种 ssrf 攻击的感觉

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

那可能先关闭 enableUnpkg 比较合适。

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

感觉可以有个 middleware，然后读取某个远程配置，判断包名

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@github-actions github-actions[bot] github-actions[bot] left review comments

@elrrrrrrr elrrrrrrr elrrrrrrr left review comments

@popomore popomore popomore left review comments

@atian25 atian25 atian25 left review comments

Assignees
No one assigned
Labels
enhancement New feature or request released
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support list and view raw files
5 participants
@fengmk2 @elrrrrrrr @baxtergu @atian25 @popomore