From bed232af86ac5a271aa8aa369af548d940d962d8 Mon Sep 17 00:00:00 2001 From: Moritz Kiefer Date: Thu, 22 Jul 2021 09:17:53 +0200 Subject: [PATCH 1/2] Limit length of package ids to 64 characters MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In practice, you cannot use a package id that is not a valid sha256 outside of a test anyway since we validate them. However, we didn’t enforce it at the daml-lf level which is a bit annoying. This PR fixes this and adds a length restriction of 64 characters (i.e. 256 bytes for sha256). We could go a bit further and also restrict the characters to valid sha256 hashes (i.e. 64 character hex strings). I don’t feel all that strongly about that either way. We use other characters in tests but fixing that shouldn’t be particularly hard either. The extra characters don’t seem to cause problem so far, so I think it’s fine to keep that. No changelog entry since I don’t see how a user can be affected by this. changelog_begin changelog_end --- .../com/digitalasset/daml/lf/data/IdString.scala | 2 +- .../scala/com/digitalasset/daml/lf/data/RefTest.scala | 2 ++ .../daml/lf/archive/testing/DamlLfEncoder.scala | 2 +- .../digitalasset/daml/lf/speedy/RollbackTest.scala | 2 +- daml-lf/spec/daml-lf-1.rst | 11 ++--------- .../main/scala/lf/value/test/ValueGenerators.scala | 2 +- 6 files changed, 8 insertions(+), 13 deletions(-) diff --git a/daml-lf/data/src/main/scala/com/digitalasset/daml/lf/data/IdString.scala b/daml-lf/data/src/main/scala/com/digitalasset/daml/lf/data/IdString.scala index 14482499e61a..b8178cb842a1 100644 --- a/daml-lf/data/src/main/scala/com/digitalasset/daml/lf/data/IdString.scala +++ b/daml-lf/data/src/main/scala/com/digitalasset/daml/lf/data/IdString.scala @@ -307,7 +307,7 @@ private[data] final class IdStringImpl extends IdString { */ override type PackageId = String override val PackageId: ConcatenableStringModule[PackageId, HexString] = - new ConcatenableMatchingStringModule("Daml-LF Package ID", "-_ ") + new ConcatenableMatchingStringModule("Daml-LF Package ID", "-_ ", 64) /** Used to reference to leger objects like contractIds, ledgerIds, * transactionId, ... We use the same type for those ids, because we diff --git a/daml-lf/data/src/test/scala/com/digitalasset/daml/lf/data/RefTest.scala b/daml-lf/data/src/test/scala/com/digitalasset/daml/lf/data/RefTest.scala index 81bf2f6b3228..1babd2a49fec 100644 --- a/daml-lf/data/src/test/scala/com/digitalasset/daml/lf/data/RefTest.scala +++ b/daml-lf/data/src/test/scala/com/digitalasset/daml/lf/data/RefTest.scala @@ -204,6 +204,8 @@ class RefTest extends AnyFreeSpec with Matchers with EitherValues { "reject too long string" in { Party.fromString("p" * 255) shouldBe a[Right[_, _]] Party.fromString("p" * 256) shouldBe a[Left[_, _]] + PackageId.fromString("p" * 64) shouldBe a[Right[_, _]] + PackageId.fromString("p" * 65) shouldBe a[Left[_, _]] } } diff --git a/daml-lf/encoder/src/main/scala/com/digitalasset/daml/lf/archive/testing/DamlLfEncoder.scala b/daml-lf/encoder/src/main/scala/com/digitalasset/daml/lf/archive/testing/DamlLfEncoder.scala index bcd12af5d326..9dbc2f4c3bc1 100644 --- a/daml-lf/encoder/src/main/scala/com/digitalasset/daml/lf/archive/testing/DamlLfEncoder.scala +++ b/daml-lf/encoder/src/main/scala/com/digitalasset/daml/lf/archive/testing/DamlLfEncoder.scala @@ -29,7 +29,7 @@ private[daml] object DamlLfEncoder extends App { throw new Error("You should not get this error") } - private val pkgId = Ref.PackageId.assertFromString("-self-") + private val pkgId = Ref.PackageId.assertFromString("self") private def main() = try { diff --git a/daml-lf/interpreter/src/test/scala/com/digitalasset/daml/lf/speedy/RollbackTest.scala b/daml-lf/interpreter/src/test/scala/com/digitalasset/daml/lf/speedy/RollbackTest.scala index c895268dbaa9..e1a4ea59619a 100644 --- a/daml-lf/interpreter/src/test/scala/com/digitalasset/daml/lf/speedy/RollbackTest.scala +++ b/daml-lf/interpreter/src/test/scala/com/digitalasset/daml/lf/speedy/RollbackTest.scala @@ -30,7 +30,7 @@ class ExceptionTest extends AnyWordSpec with Matchers with TableDrivenPropertyCh implicit val defaultParserParameters: ParserParameters[this.type] = { ParserParameters( - defaultPackageId = Ref.PackageId.assertFromString("-pkgId-"), + defaultPackageId = Ref.PackageId.assertFromString("pkgId"), languageVersion = LanguageVersion.v1_dev, ) } diff --git a/daml-lf/spec/daml-lf-1.rst b/daml-lf/spec/daml-lf-1.rst index 58510681e5a4..10448fe8625d 100644 --- a/daml-lf/spec/daml-lf-1.rst +++ b/daml-lf/spec/daml-lf-1.rst @@ -375,17 +375,10 @@ instances when we want to avoid empty identifiers, escaping problems, and other similar pitfalls. :: PackageId strings - PackageIdString ::= ' PackageIdChars ' -- PackageIdString - - Sequences of PackageId character - PackageIdChars ::= PackageIdChar -- PackageIdChars - | PackageIdChars PackageIdChar - - PackageId character - PackageIdChar ∈ [a-zA-Z0-9\-_ ] -- PackageIdChar + PackageIdString ::= [a-zA-Z0-9\-_ ]{1,64} -- PackageIdString PartyId strings - PartyIdString ∈ [a-zA-Z0-9:\-_ ]{1,255} -- PartyIdChar + PartyIdString ∈ [a-zA-Z0-9:\-_ ]{1,255} -- PartyIdString PackageName strings PackageNameString ∈ [a-zA-Z0-9:\-_]+ -- PackageNameString diff --git a/daml-lf/transaction-test-lib/src/main/scala/lf/value/test/ValueGenerators.scala b/daml-lf/transaction-test-lib/src/main/scala/lf/value/test/ValueGenerators.scala index 3dbd23f967e9..9fee3ee0d327 100644 --- a/daml-lf/transaction-test-lib/src/main/scala/lf/value/test/ValueGenerators.scala +++ b/daml-lf/transaction-test-lib/src/main/scala/lf/value/test/ValueGenerators.scala @@ -77,7 +77,7 @@ object ValueGenerators { // generate a junk identifier val idGen: Gen[Identifier] = for { - n <- Gen.choose(1, 200) + n <- Gen.choose(1, 64) packageId <- Gen .listOfN(n, Gen.alphaNumChar) .map(s => PackageId.assertFromString(s.mkString)) From 2362d1e750563c4ecb5fc1e19cefabfc4c984e56 Mon Sep 17 00:00:00 2001 From: Moritz Kiefer Date: Thu, 22 Jul 2021 11:40:06 +0200 Subject: [PATCH 2/2] Update daml-lf/encoder/src/main/scala/com/digitalasset/daml/lf/archive/testing/DamlLfEncoder.scala --- .../digitalasset/daml/lf/archive/testing/DamlLfEncoder.scala | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/daml-lf/encoder/src/main/scala/com/digitalasset/daml/lf/archive/testing/DamlLfEncoder.scala b/daml-lf/encoder/src/main/scala/com/digitalasset/daml/lf/archive/testing/DamlLfEncoder.scala index 9dbc2f4c3bc1..bcd12af5d326 100644 --- a/daml-lf/encoder/src/main/scala/com/digitalasset/daml/lf/archive/testing/DamlLfEncoder.scala +++ b/daml-lf/encoder/src/main/scala/com/digitalasset/daml/lf/archive/testing/DamlLfEncoder.scala @@ -29,7 +29,7 @@ private[daml] object DamlLfEncoder extends App { throw new Error("You should not get this error") } - private val pkgId = Ref.PackageId.assertFromString("self") + private val pkgId = Ref.PackageId.assertFromString("-self-") private def main() = try {