feat(gnovm): forbid importing realms in packages #3042

MikaelVallenet · 2024-10-28T21:13:47Z

Closes #3040
80% of the work comes from @harry-hov's PR #1393 (let's repay to Caesar what belongs to Caesar) 🚀

Notable additions:

handle different domains (e.g github.com/p/demo/...)
skip non .gno files (LICENSE, README, ...) or empty files

Contributors' checklist...

Added new tests, or not needed, or not feasible
Provided an example (e.g. screenshot) to aid review or the PR is self-explanatory
Updated the official documentation or not needed
No breaking changes were made, or a BREAKING CHANGE: xxx message was included in the description
Added references to related issues and PRs
Provided any useful hints for running manual tests

codecov · 2024-10-28T21:15:56Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 63.32%. Comparing base (af05780) to head (9ff76f7).

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3042      +/-   ##
==========================================
- Coverage   63.32%   63.32%   -0.01%     
==========================================
  Files         548      548              
  Lines       78511    78528      +17     
==========================================
+ Hits        49719    49729      +10     
- Misses      25438    25447       +9     
+ Partials     3354     3352       -2

Flag	Coverage Δ
contribs/gnodev	`60.57% <ø> (ø)`
contribs/gnofaucet	`14.82% <ø> (ø)`
gno.land	`67.18% <ø> (ø)`
gnovm	`67.90% <100.00%> (+0.01%)`	⬆️
misc/genstd	`79.72% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

…ards

omarsy

It might be a good idea to include additional tests, such as checking what happens when we publish a package or testing case. WDYT?

n0izn0iz

logic LGTM, optimization comments since it's part of consensus

gnovm/memfile.go

n0izn0iz · 2024-10-29T20:02:36Z

gnovm/memfile.go

+				continue // can be other files like LICENSE, README or empty gno files
+			}
+			for _, imp := range astFile.Imports {
+				// ensure the pkg is a realm by checking if the path contains /r/ and no other / character before it (i.e protect from gno.land/p/demo/r/)


maybe we should add a test for the case gno.land/p/demo/r/

gnovm/memfile.go

Co-authored-by: n0izn0iz <[email protected]>

thehowl · 2024-10-30T20:03:01Z

gno.land/cmd/gnoland/testdata/assertorigincall.txtar

-## 16. MsgRun -> run.main -> bar.A: PANIC
-! gnokey maketx run -gas-fee 100000ugnot -gas-wanted 4000000 -broadcast -chainid tendermint_test test1 $WORK/run/barA.gno
-stderr 'invalid non-origin call'
-
-## 17. MsgRun -> run.main -> bar.B: PASS
-gnokey maketx run -gas-fee 100000ugnot -gas-wanted 4000000 -broadcast -chainid tendermint_test test1 $WORK/run/barB.gno
-stdout 'OK!'


Why did these get removed?

Msgrun should be able to import packages.

I remove it because it tested the flow: msgRun -> run.main -> package -> realm
But indeed, i should have keep the flow with just removing the link to the realm
fix here: 21f15fe

jefft0 · 2024-10-31T08:35:04Z

Hello @MikaelVallenet . Please merge master. This should fix the CI check errors for "unknown revision v0.1.1".

…t/forbid-importing-realms

Co-authored-by: n0izn0iz <[email protected]>

…t/forbid-importing-realms

n0izn0iz

we should probably add tests cases with

non-gno file
invalid gno file that will trigger a parser error

Also I feel this PR should not remove the MsgCall tests since it does not change this behavior. Think we may add them back with a package that does not import a realm. But it's not very important

n0izn0iz · 2024-10-31T17:08:30Z

gno.land/cmd/gnoland/testdata/prevrealm.txtar

+# | 11  |          | through /p/demo/bar | bar.A()         | user address |
+# | 12  |          |                     | bar.B()         | user address |
+# | 9   | MsgCall  | wallet direct       | std.PrevRealm() | user address |
+# | 10  | MsgRun   | wallet direct       | std.PrevRealm() | user address |


order and index do not match the actual tests below

…le skip

MikaelVallenet · 2024-10-31T21:16:27Z

we should probably add tests cases with

non-gno file

invalid gno file that will trigger a parser error

Also I feel this PR should not remove the MsgCall tests since it does not change this behavior. Think we may add them back with a package that does not import a realm. But it's not very important

i added test 6081a7b
In case of the MsgCall i deleted, they were comments so i thought it does need to stay here
I could adapt them to do like pkg -> realm, but i'm not sure it would be a useful thing

…t/forbid-importing-realms

n0izn0iz · 2024-10-31T22:58:28Z

what I meant is that the MsgCall tests that call into a package should probably be removed by the actual PR that will prevent MsgCall into /p/

we could keep them like the MsgRun tests but with a /p/ that does not imports a /r/

I'm not sure this is very important so if it's a pain nevermind

~~the test summary tables and actual tests seems still off, for example, test 8 in assertorigincall.txtar is a MsgRun type but it's not marked as such in the summary~~

MikaelVallenet · 2024-11-01T09:42:55Z

what I meant is that the MsgCall tests that call into a package should probably be removed by the actual PR that will prevent MsgCall into /p/

we could keep them like the MsgRun tests but with a /p/ that does not imports a /r/

I'm not sure this is very important so if it's a pain nevermind

the test summary tables and actual tests seems still off, for example, test 8 in assertorigincall.txtar is a MsgRun type but it's not marked as such in the summary @n0izn0iz

I think i get your point -> 9ff76f7 wdyt

MikaelVallenet added 2 commits October 28, 2024 21:51

fix: forbid importing realms from pure packages r/ from p/

f2fb5f7

fix: handle different domain name

112daa1

github-actions bot assigned MikaelVallenet Oct 28, 2024

github-actions bot added the 📦 🤖 gnovm Issues or PRs gnovm related label Oct 28, 2024

MikaelVallenet added 5 commits October 28, 2024 22:19

fix: use index instead of splitting the path

144f4a2

fix: remove redundant condition

29d018e

fix: check there is no slash before instead of looking for one

4b733e3

fix: run golangci-lint

9f6f5cb

test: adapt assert origin call txtar test

1f12e19

github-actions bot added the 📦 ⛰️ gno.land Issues or PRs gno.land package related label Oct 29, 2024

MikaelVallenet added 3 commits October 29, 2024 15:11

fix: skip unparsable file

aab9d55

test: adapt prevrealm test txtar

4fbe93d

fix: remove groups file in groups pkg since it's just a wrapper of bo…

8f61825

…ards

github-actions bot added the 🧾 package/realm Tag used for new Realms or Packages. label Oct 29, 2024

MikaelVallenet added 15 commits October 29, 2024 16:28

chore: run gno mod tidy

d2f0200

fix: remove call from realm into pkg demo/test

7b026d1

test: add txtar testfile to import realm into pkg

e26a293

fix: remove testfile about realm to pkg import

91aa244

fix: adapt zrealm_crossrealm11 test to new import restriction

794b5a9

fix: txtar prevrealm & import realm into pkg

a54975d

fix: txtat import realm pkg

da09313

fix: trying to fix txtar test

d52969c

fix: remove txtar not working test file

eccc2e4

fix: add test import about pkg importing realm not working

a0fc9f6

fix: add test import about pkg importing realm not working

0fc525c

fix: add test import about pkg importing realm not working

49828e8

fix: add test import about pkg importing realm not working

9af0e68

fix: add test import about pkg importing realm not working

5459c2d

fix: add test import about pkg importing realm not working

c92d714

Merge branch 'master' into dev/mikaelvallenet/forbid-importing-realms

b28d1bd

Kouteki added the review/triage-pending PRs opened by external contributors that are waiting for the 1st review label Oct 29, 2024

omarsy reviewed Oct 29, 2024

View reviewed changes

n0izn0iz reviewed Oct 29, 2024

View reviewed changes

MikaelVallenet and others added 3 commits October 30, 2024 14:42

chore: optimization

7116ef1

Co-authored-by: n0izn0iz <[email protected]>

chore: optimization

90190a0

Co-authored-by: n0izn0iz <[email protected]>

fix: add test for handling /r/ as a realm name or namespace

7b64799

thehowl reviewed Oct 30, 2024

View reviewed changes

MikaelVallenet and others added 6 commits October 31, 2024 12:54

Merge remote-tracking branch 'upstream/master' into dev/mikaelvallene…

c5512bc

…t/forbid-importing-realms

fix(gnovom): optimize

ad1c8df

Co-authored-by: n0izn0iz <[email protected]>

fix(gnovm): return error on parsing error

cf90c37

fix(gnovm): add body to testfiles to avoid crash on test parsing

dd1bd94

fix(gnovm): add pkg import in assertorigincall txtar

21f15fe

fix(gnovm): add pkg import from msgrun in prevrealm txtar

128f7f4

MikaelVallenet requested review from thehowl, n0izn0iz and omarsy October 31, 2024 14:09

Merge remote-tracking branch 'upstream/master' into dev/mikaelvallene…

530edfb

…t/forbid-importing-realms

n0izn0iz suggested changes Oct 31, 2024

View reviewed changes

MikaelVallenet added 3 commits October 31, 2024 22:00

fix(gnovm): re order index in prevrealm txtar

63be6b3

test(gnovm): add two test case file about empty gno file & non gno fi…

6081a7b

…le skip

test(gnovm): fix order

8d97af3

MikaelVallenet requested a review from n0izn0iz October 31, 2024 21:16

Merge remote-tracking branch 'upstream/master' into dev/mikaelvallene…

8f0bbc1

…t/forbid-importing-realms

fix: keep commented tests

9ff76f7

zxxma mentioned this pull request Nov 1, 2024

Teritori's Builder Journey gnolang/hackerspace#7

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(gnovm): forbid importing realms in packages #3042

feat(gnovm): forbid importing realms in packages #3042

MikaelVallenet commented Oct 28, 2024 •

edited by thehowl

Loading

codecov bot commented Oct 28, 2024 •

edited

Loading

omarsy left a comment •

edited

Loading

n0izn0iz left a comment

n0izn0iz Oct 29, 2024

MikaelVallenet Oct 30, 2024

thehowl Oct 30, 2024

MikaelVallenet Oct 31, 2024

jefft0 commented Oct 31, 2024

n0izn0iz left a comment •

edited

Loading

n0izn0iz Oct 31, 2024

MikaelVallenet commented Oct 31, 2024

n0izn0iz commented Oct 31, 2024 •

edited

Loading

MikaelVallenet commented Nov 1, 2024

feat(gnovm): forbid importing realms in packages #3042

Are you sure you want to change the base?

feat(gnovm): forbid importing realms in packages #3042

Conversation

MikaelVallenet commented Oct 28, 2024 • edited by thehowl Loading

codecov bot commented Oct 28, 2024 • edited Loading

Codecov Report

omarsy left a comment • edited Loading

Choose a reason for hiding this comment

n0izn0iz left a comment

Choose a reason for hiding this comment

n0izn0iz Oct 29, 2024

Choose a reason for hiding this comment

MikaelVallenet Oct 30, 2024

Choose a reason for hiding this comment

thehowl Oct 30, 2024

Choose a reason for hiding this comment

MikaelVallenet Oct 31, 2024

Choose a reason for hiding this comment

jefft0 commented Oct 31, 2024

n0izn0iz left a comment • edited Loading

Choose a reason for hiding this comment

n0izn0iz Oct 31, 2024

Choose a reason for hiding this comment

MikaelVallenet commented Oct 31, 2024

n0izn0iz commented Oct 31, 2024 • edited Loading

MikaelVallenet commented Nov 1, 2024

MikaelVallenet commented Oct 28, 2024 •

edited by thehowl

Loading

codecov bot commented Oct 28, 2024 •

edited

Loading

omarsy left a comment •

edited

Loading

n0izn0iz left a comment •

edited

Loading

n0izn0iz commented Oct 31, 2024 •

edited

Loading