search

readium / readium-lcp-server

Repository for the Readium LCP Server
BSD 3-Clause "New" or "Revised" License
73 stars 58 forks source link

Fix in Generate a license #192

Closed xabierXercode closed 9 months ago

xabierXercode commented 5 years ago

When create a partial license with user encrypted field different of name or email the server throws error:

[negroni] PANIC: reflect: call of reflect.Value.Set on zero Value
goroutine 21 [running]:
github.com/urfave/negroni.(*Recovery).ServeHTTP.func1(0x7f628ee0c0f0, 0xc00009e058, 0xc00018c8c0, 0xc00013c100)
    /workspace/go-work/src/github.com/urfave/negroni/recovery.go:161 +0xef
panic(0xb2ef20, 0xc0003b7120)
    /usr/local/go/src/runtime/panic.go:513 +0x1b9
reflect.flag.mustBeAssignable(0x0)
    /usr/local/go/src/reflect/value.go:227 +0xc0
reflect.Value.Set(0x0, 0x0, 0x0, 0xaf0580, 0xc000295010, 0x98)
    /usr/local/go/src/reflect/value.go:1397 +0x2f

The request: 

{
    "provider": "https://xeread.xebook.es",
    "user": {
      "id": "d9f298a7-7f34-49e7-8aae-4378ecb1d597",
      "email": "demo@xercode.es",
      "library": "Xebook",
      "encrypted": ["email", "library"]

    },
    "encryption": {
      "user_key": {
         "text_hint": "Enter the username address which authentifies you on xeread.xebook.es",
         "hex_value": "4981AA0A50D563040519E9032B5D74367B1D129E239A1BA82667A57333866494",
         "algorithm": "http://www.w3.org/2001/04/xmlenc#sha256"        
      }
    },
     "rights": {
        "print": 10,
        "copy": 2048,
        "start": "2019-03-29T01:08:15+01:00",
        "end": "2019-04-29T01:08:15+01:00"
    }
  }

The error occurs in : https://github.com/readium/readium-lcp-server/blob/master/license/license.go#L235

The solution to this problem is easy:


        if (!field.IsValid()) {
            return  fmt.Errorf("The field '%s' is not valid for encrypted. The valid fields are email and name.", toEncrypt);
        }
danielweck commented 5 years ago

Well, UserInfo does not support the field library, only id, email and name: https://github.com/readium/readium-lcp-server/blob/c72c003c861c70f66fbc12d3745ff78a79ea98d0/license/license.go#L62-L67

That being said, the LCP specification allows arbitrary vendor extensions (i.e. custom JSON fields / properties / keys): https://readium.org/technical/readium-lcp-specification/#37-identifying-the-user-the-user-object

...so your library field should in fact be a URI (am I right, @HadrienGardeur @llemeurfr ?).

More importantly, the Go server code should be capable of handling these arbitrary keys, notably when there is a request to encrypt them in the generated license JSON. See the getField function: https://github.com/readium/readium-lcp-server/blob/c72c003c861c70f66fbc12d3745ff78a79ea98d0/license/license.go#L245-L248

So based on my quick investigation, I would say "thank you for the bug report!" :) but I would also say "let's not use the suggested if (!field.IsValid()) check, instead let's fix this properly by implementing support for encrypting vendor extension URIs / custom fields".

What do you think @xabierXercode ?

xabierXercode commented 5 years ago

@danielweck I agree that we should implement support for support to encrypt URI of provider extension / custom fields.

I think we must also make the server more robust. We must not allow fields in the petition that do not adhere to the specification. I think we should also send some kind of error when this occurs. Maybe the right place to do this will be in: https://github.com/readium/readium-lcp-server/blob/c72c003c861c70f66fbc12d3745ff78a79ea98d0/lcpserver/api/license.go#L82

What do you think?

llemeurfr commented 5 years ago

I agree that this will have to be fixed properly to allow provider extensions and trap the kind of error you've encountered.

llemeurfr commented 2 years ago

Since that time, the code was hardened to accept a partial license with unknown fields. But the convertion from JSON to an object is still strict and unknown fields are lost during the process.

mpdunlop commented 1 year ago

The issue appears to still be occurring. The error:

[negroni] PANIC: reflect: call of reflect.Value.Set on zero Value
goroutine 167 [running]:
github.com/urfave/negroni.(*Recovery).ServeHTTP.func1()
        C:/Users/matthew.dunlop/go/pkg/mod/github.com/urfave/negroni@v1.0.0/recovery.go:159 +0xad
panic({0x7ff6dead5840?, 0xc000538528?})
        C:/Program Files/Go/src/runtime/panic.go:914 +0x21f
reflect.flag.mustBeAssignableSlow(0x7ff6dea98da0?)
        C:/Program Files/Go/src/reflect/value.go:265 +0x105
reflect.flag.mustBeAssignable(...)
        C:/Program Files/Go/src/reflect/value.go:259
reflect.Value.Set({0x0?, 0x0?, 0x20?}, {0x7ff6dea98720?, 0xc000037220?, 0xc0000f4d80?})
        C:/Program Files/Go/src/reflect/value.go:2254 +0x65
github.com/readium/readium-lcp-server/license.encryptFields({0x7ff6deea0e00, 0x7ff6df481440}, 0xc0000b6840, {0xc0005305c0, 0x20, 0x40})
        C:/Users/matthew.dunlop/go/pkg/mod/github.com/readium/readium-lcp-server@v0.0.0-20230809161937-3d23a1331703/license/license.go:348 +0x85
github.com/readium/readium-lcp-server/license.EncryptLicenseFields(0xc0000b6840, {{0xc00000b588, 0x8}, {0xc000020520, 0x20, 0x20}, {0xc000024320, 0x3e}, 0xcc8ac, {0xc000024370, ...}, ...})
        C:/Users/matthew.dunlop/go/pkg/mod/github.com/readium/readium-lcp-server@v0.0.0-20230809161937-3d23a1331703/license/license.go:319 +0x1b9
github.com/readium/readium-lcp-server/lcpserver/api.buildLicense(0xc0000b6840, {0x7ff6deea25c0, 0xc0000d6120}, 0x0)
        C:/Users/matthew.dunlop/go/pkg/mod/github.com/readium/readium-lcp-server@v0.0.0-20230809161937-3d23a1331703/lcpserver/api/license.go:138 +0x205
github.com/readium/readium-lcp-server/lcpserver/api.GenerateLicense({0x241f5fe57f8?, 0xc000520020}, 0xc000520020?, {0x7ff6deea25c0?, 0xc0000d6120})
        C:/Users/matthew.dunlop/go/pkg/mod/github.com/readium/readium-lcp-server@v0.0.0-20230809161937-3d23a1331703/lcpserver/api/license.go:366 +0x11b
github.com/readium/readium-lcp-server/lcpserver/server.New.(*Server).handlePrivateFunc.func5({0x241f5fe57f8, 0xc000520020}, 0xc0000f49f0?)
        C:/Users/matthew.dunlop/go/pkg/mod/github.com/readium/readium-lcp-server@v0.0.0-20230809161937-3d23a1331703/lcpserver/server/server.go:156 +0x6f
net/http.HandlerFunc.ServeHTTP(0xc00007ca00?, {0x241f5fe57f8?, 0xc000520020?}, 0x7ff6de28ce65?)
        C:/Program Files/Go/src/net/http/server.go:2136 +0x29
github.com/gorilla/mux.(*Router).ServeHTTP(0xc0000d2000, {0x241f5fe57f8, 0xc000520020}, 0xc00007c900)
        C:/Users/matthew.dunlop/go/pkg/mod/github.com/gorilla/mux@v1.8.0/mux.go:210 +0x1c5
github.com/urfave/negroni.(*Negroni).UseHandler.Wrap.func1({0x241f5fe57f8, 0xc000520020}, 0x7ff6deb0f460?, 0xc000065120)
        C:/Users/matthew.dunlop/go/pkg/mod/github.com/urfave/negroni@v1.0.0/negroni.go:46 +0x45
github.com/urfave/negroni.HandlerFunc.ServeHTTP(0x7ff6de189628?, {0x241f5fe57f8?, 0xc000520020?}, 0x0?, 0x7ff6de3f19b9?)
        C:/Users/matthew.dunlop/go/pkg/mod/github.com/urfave/negroni@v1.0.0/negroni.go:29 +0x2d
github.com/urfave/negroni.middleware.ServeHTTP({{0x7ff6dee9cae0?, 0xc000538300?}, 0xc000538378?}, {0x241f5fe57f8, 0xc000520020}, 0xc0000b6768?)
        C:/Users/matthew.dunlop/go/pkg/mod/github.com/urfave/negroni@v1.0.0/negroni.go:38 +0xa8
github.com/rs/cors.(*Cors).ServeHTTP(0xc0000d20c0, {0x241f5fe57f8, 0xc000520020}, 0xc00007c900, 0xc000065100)
        C:/Users/matthew.dunlop/go/pkg/mod/github.com/rs/cors@v1.8.2/cors.go:266 +0x17e
github.com/urfave/negroni.middleware.ServeHTTP({{0x7ff6dee9c2e0?, 0xc0000d20c0?}, 0xc000538360?}, {0x241f5fe57f8, 0xc000520020}, 0x7ff6de25c68f?)
        C:/Users/matthew.dunlop/go/pkg/mod/github.com/urfave/negroni@v1.0.0/negroni.go:38 +0xa8
github.com/urfave/negroni.(*Logger).ServeHTTP(0xc00009a8a0, {0x241f5fe57f8?, 0xc000520020}, 0xc00007c900, 0xc0000650e0)
        C:/Users/matthew.dunlop/go/pkg/mod/github.com/urfave/negroni@v1.0.0/logger.go:62 +0x83
github.com/urfave/negroni.middleware.ServeHTTP({{0x7ff6dee9c2a0?, 0xc00009a8a0?}, 0xc000538348?}, {0x241f5fe57f8, 0xc000520020}, 0x7ff6deb0f460?)
        C:/Users/matthew.dunlop/go/pkg/mod/github.com/urfave/negroni@v1.0.0/negroni.go:38 +0xa8
github.com/urfave/negroni.(*Recovery).ServeHTTP(0xc0004c19a0?, {0x241f5fe57f8?, 0xc000520020?}, 0x5007ff6de465b8e?, 0xc13dbaf4234a38f0?)
        C:/Users/matthew.dunlop/go/pkg/mod/github.com/urfave/negroni@v1.0.0/recovery.go:193 +0x7b
github.com/urfave/negroni.middleware.ServeHTTP({{0x7ff6dee9c280?, 0xc000526690?}, 0xc000538330?}, {0x241f5fe57f8, 0xc000520020}, 0xb?)
        C:/Users/matthew.dunlop/go/pkg/mod/github.com/urfave/negroni@v1.0.0/negroni.go:38 +0xa8
github.com/jeffbmartinez/delay.Middleware.ServeHTTP({}, {0x241f5fe57f8, 0xc000520020}, 0xc00007c900, 0xc000065080)
        C:/Users/matthew.dunlop/go/pkg/mod/github.com/jeffbmartinez/delay@v0.0.0-20150608194421-e1b689d78b33/delay.go:23 +0x68
github.com/urfave/negroni.middleware.ServeHTTP({{0x7ff6dee9c260?, 0x7ff6df481440?}, 0xc000538318?}, {0x241f5fe57f8, 0xc000520020}, 0x7ff6deb0d2e0?)
        C:/Users/matthew.dunlop/go/pkg/mod/github.com/urfave/negroni@v1.0.0/negroni.go:38 +0xa8
github.com/urfave/negroni.(*Negroni).ServeHTTP(0xc00009a810, {0x7ff6deea1410?, 0xc0000b21c0}, 0xc0000b2101?)
        C:/Users/matthew.dunlop/go/pkg/mod/github.com/urfave/negroni@v1.0.0/negroni.go:96 +0x114
net/http.serverHandler.ServeHTTP({0x7ff6dee9d9b0?}, {0x7ff6deea1410?, 0xc0000b21c0?}, 0x6?)
        C:/Program Files/Go/src/net/http/server.go:2938 +0x8e
net/http.(*conn).serve(0xc0001be000, {0x7ff6deea1d70, 0xc0000f4390})
        C:/Program Files/Go/src/net/http/server.go:2009 +0x5f4
created by net/http.(*Server).Serve in goroutine 1
        C:/Program Files/Go/src/net/http/server.go:3086 +0x5cb

The request:

{
    "provider": "http://www.imaginaryebookretailer.com",
    "user": {
        "id": "d9f298a7-7f34-49e7-8aae-4378ecb1d597",
        "email": "user@imaginaryebookretailer.com",
        "http://www.imaginaryebookretailer.com/lcp/vendorspecificvalue": "f7a02b00-952e-44d9-a952-6b2c553f5242",
        "encrypted": [
            "email",
            "http://www.imaginaryebookretailer.com/lcp/vendorspecificvalue"
        ]
    },
    "encryption": {
        "user_key": {
            "text_hint": "The title of the first book you ever read",
            "hex_value": "4981AA0A50D563040519E9032B5D74367B1D129E239A1BA82667A57333866494"
        }
    },
    "rights": {
        "print": 10,
        "copy": 2048
    }
}

lcpserver can accept partial licenses with unknown fields but can't deal with encrypting them, instead printing an exception to console and returning the stack trace in the response body.

llemeurfr commented 9 months ago

That's right: the custom field is filtered during json unmarshaling, but its reference in the encrypted array attribute is kept in the resulting array. At the time the fields are encrypted, a null pointer is generated and processed without care.

At this point in the development of the LCP Server v1, allowing custom fields in partial licenses seems a bit complex, as we'd need to code a custom JSON decoder working on random JSON fields. It should be simpler in the LCP Server v2 by creating an extension point in license requests, a generic meta field with name and value attributes, and some custom json marshalling code for the license.

For now I'll create a fix where the custom field is removed from both the user array and the encrypted property in the returned license.

llemeurfr commented 9 months ago

Fixed in e0d40746470c9f2cee03b23b4755af24f03702dd